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=-7.4 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MIME_HTML_MOSTLY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 78FF3C433E0 for ; Fri, 19 Feb 2021 05:44:52 +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 0E58F64EC7 for ; Fri, 19 Feb 2021 05:44:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0E58F64EC7 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 339466EA9D; Fri, 19 Feb 2021 05:44:46 +0000 (UTC) Received: from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com [IPv6:2a00:1450:4864:20::52e]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8054B6E43C; Fri, 19 Feb 2021 05:44:44 +0000 (UTC) Received: by mail-ed1-x52e.google.com with SMTP id p2so7604113edm.12; Thu, 18 Feb 2021 21:44:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=3M7g8PXzqD5fEc/XC6jIred3VjK7nnn29ZoJzRmaMnI=; b=S1j5RrGgN55SRnuU0wIZxT23P4JFKh3MHp8zeFuEMcbt1kJwFu5RXMC7poQeUAPMfk gZVIrRqGLSyyfrVUhNyf4ro70AmTfCEfYBh7SUjQGxmQ0z+SR1UTVwL2Zjq+wXdhCz+I LoiR8YnvbDhuSjCtoKVo0z1dE4cIUZ6x21UshBNWlURtesKvG9MPuu/pmmNwcl25Glb3 NrH+1dG8SO3gRpjPa/+olv4UavMcIwbeuNHsHTHhN4/k3LewBD65EGp91ah3rQjwNYZB QuSzN18Wlv7zD11m8LCfVWlVpbkcU8jGjq8SmPkU7AsSp3XpEf0jNBqmWJ4fPraCp61q Nr8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=3M7g8PXzqD5fEc/XC6jIred3VjK7nnn29ZoJzRmaMnI=; b=tvpUp8Cv3tHTMBHu4p4E5JN+ItaAd1lOK5j8N5CIU/ZDkqWanMJsHwSJ04J1CWy3QZ RVdlaRtbw2fzYv+wbZCIBRWAhkvDQFa9TSB7plWHrwNbU7+HIkre9JL/o1eRCRQ09G74 hFkxDuXkMclNZCMDrQMoa6vLbt0CMePQwThkDwDGQPPTiSKijiq6v72sn+LAHN7BrTRr bMRzKYL2YppSDSt61Yl4HYoaeOpm4alXzuW7iJevL2nvqKIAljORnKxwgBo4NFmga6aw mZXjmjIN8OI+JnosV6fEFHppW5NH5+/writzrsjAHrptLgccA7U28c2nMuit/5ct5eZ7 ukvw== X-Gm-Message-State: AOAM530IjtJvIY/a3jChAXd2KEenna7R1a64ZXYECYpSIyiVFxtDMn3S R1g+0f6FiFOEtmIPVgh1n6+zZ6O4kKlFAhZBtdrE4qO+7/g= X-Google-Smtp-Source: ABdhPJzdGh7tSOgUYzbDZ37RMNDFJXB4uz1jA+Lw00GC6Izgk3BkRKW3CrUyq54A/4z2nTFLTmmCtpM3rEHyj9qfZ1A= X-Received: by 2002:a05:6402:1bc7:: with SMTP id ch7mr7717288edb.84.1613713483071; Thu, 18 Feb 2021 21:44:43 -0800 (PST) MIME-Version: 1.0 References: <20210128192413.1715802-1-matthew.d.roper@intel.com> <20210128192413.1715802-19-matthew.d.roper@intel.com> In-Reply-To: From: Mario Kleiner Date: Fri, 19 Feb 2021 06:44:31 +0100 Message-ID: To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= Subject: Re: [Intel-gfx] [PATCH 18/18] drm/i915/display13: Enabling dithering after the CC1 pipe X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx , Nischal Varide , dri-devel Content-Type: multipart/mixed; boundary="===============0196590124==" Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" --===============0196590124== Content-Type: multipart/alternative; boundary="000000000000ed81e305bba9f2fc" --000000000000ed81e305bba9f2fc Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Feb 19, 2021 at 4:22 AM Mario Kleiner wrote: > > > On Thu, Feb 11, 2021 at 1:29 PM Ville Syrj=C3=A4l=C3=A4 < > ville.syrjala@linux.intel.com> wrote: > >> On Thu, Jan 28, 2021 at 11:24:13AM -0800, Matt Roper wrote: >> > From: Nischal Varide >> > >> > If the panel is 12bpc then Dithering is not enabled in the Legacy >> > dithering block , instead its Enabled after the C1 CC1 pipe post >> > color space conversion.For a 6bpc pannel Dithering is enabled in >> > Legacy block. >> >> Dithering is probably going to require a whole uapi bikeshed. >> Not sure we can just enable it unilaterally. >> >> Ccing dri-devel, and Mario who had issues with dithering in the >> past... >> >> Thanks for the cc Ville! > > The problem with dithering on Intel is that various tested Intel gpu's > (Ironlake, IvyBridge, Haswell, Skylake iirc.) are dithering when they > shouldn't. If one has a standard 8 bpc framebuffer feeding into a standar= d > (legacy) 256 slots, 8 bit wide lut which was loaded with an identity > mapping, feeding into a standard 8 bpc video output (DVI/HDMI/DP), the > expected result is that pixels rendered into the framebuffer show up > unmodified at the video output. What happens instead is that some ditheri= ng > is needlessly applied. This is bad for various neuroscience/medical > research equipment that requires pixels to pass unmodified in a pure 8 bp= c > configuration, e.g., because some digital info is color-encoded in-band i= n > the rendered image to control research hardware, a la "if rgb pixel (123, > 12, 23) is detected in the digital video stream, emit some trigger signal= , > or timestamp that moment with a hw clock, or start or stop some scientifi= c > recording equipment". Also there exist specialized visual stimulators to > drive special displays with more than 12 bpc, e.g., 16 bpc, and so they > encode the 8MSB of 16 bpc color values in pixels in even columns, and the > 8LSB in the odd columns of the framebuffer. Unexpected dithering makes su= ch > equipment completely unusable. By now I must have spent months of my life= , > just trying to deal with dithering induced problems on different gpu's du= e > to hw quirks or bugs somewhere in the graphics stack. > > Atm. the intel kms driver disables dithering for anything with >=3D 8 bpc= as > a fix for this harmful hardware quirk. > > Ideally we'd have uapi that makes dithering controllable per connector > (on/off/auto, selectable depth), also in a way that those controls are > exposed as RandR output properties, easily controllable by X clients. And > some safe default in case the client can't access the properties (like I'= d > expect to happen with the dozens of Wayland compositors under the sun). > Various drivers had this over time, e.g., AMD classic kms path (if i don'= t > misremember) and nouveau, but some of it also got lost in the new atomic > kms variants, and Intel never exposed this. > > Or maybe some method that checks the values actually stored in the hw > lut's, CTM etc. and if the values suggest no dithering should be needed, > disable the dithering. E.g., if output depth is 8 bpc, one only needs > dithering if the slots in the final active hw lut do have any meaningful > values in the lower bits below the top 8 MSB, ie. if the content is > actually > 8 bpc net bit depth. > > -mario > > One cup of coffee later... I think this specific patch should be ok wrt. my use cases. The majority of the above mentioned research devices are single/dual-link DVI receivers, ie. 8 bpc video sinks. I'm only aware of one recent device that has a DisplayPort receiver who could act as a > 8 bpc video sink. See the following link for advanced examples of such devices: https://vpixx.com/our-products/video-i-o-hub/ I cannot think of a use case that would require more than 8 bits for inband signalling given that that was good enough for the last 20 years, or for encoding very high color precision content -- the 16 bpc precision that one can get out of the current even/odd pixel =3D 8 MSB + 8 LSB encoding scheme should be enough for the foreseeable future. Therefore dithering shouldn't pose a problem if it leaves the 8 MSB of each pixel color component intact, and spatial dithering as employed here usually only touches the least significant bit (or maybe the 2 LSB's?). As this patch only enables dithering on 12 bpc video sinks, if i understand pipe_bpp correctly, it could only "corrupt" one bit and leave at least the 10-11 MSB's intact, right? pipe_bpp =3D=3D 24 is the case that would really hurt a lot of researchers = if dithering would be enabled without providing good uapi or other mechanisms to prevent it. So: Acked-by: Mario Kleiner One suggestion: It would be good to also add a bit of drm_dbg_kms() logging to the new code-patch, so that this 12 bpc dithering enable on HAS_DISPLAY13 hw also shows up in the logs, not just the standard 6 bpc enable. Helped a lot in debugging dithering issues if there was a reliable trace in the logs of what was active when. One suggestion for that inside your patch below... > >> > Cc: Uma Shankar >> > Signed-off-by: Nischal Varide >> > Signed-off-by: Bhanuprakash Modem >> > Signed-off-by: Matt Roper >> > --- >> > drivers/gpu/drm/i915/display/intel_color.c | 16 ++++++++++++++++ >> > drivers/gpu/drm/i915/display/intel_display.c | 9 ++++++++- >> > drivers/gpu/drm/i915/i915_reg.h | 3 ++- >> > 3 files changed, 26 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c >> b/drivers/gpu/drm/i915/display/intel_color.c >> > index ff7dcb7088bf..9a0572bbc5db 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_color.c >> > +++ b/drivers/gpu/drm/i915/display/intel_color.c >> > @@ -1604,6 +1604,20 @@ static u32 icl_csc_mode(const struct >> intel_crtc_state *crtc_state) >> > return csc_mode; >> > } >> > >> > +static u32 dither_after_cc1_12bpc(const struct intel_crtc_state >> *crtc_state) >> > +{ >> > + u32 gamma_mode =3D crtc_state->gamma_mode; >> > + struct drm_i915_private *i915 =3D >> to_i915(crtc_state->uapi.crtc->dev); >> > + >> > + if (HAS_DISPLAY13(i915)) { >> > + if (!crtc_state->dither_force_disable && >> > Replace !crtc_state->dither_force_disable by crtc_state->dither > > + (crtc_state->pipe_bpp =3D=3D 36)) >> > + gamma_mode |=3D GAMMA_MODE_DITHER_AFTER_CC1; >> > + } >> > + >> > + return gamma_mode; >> > +} >> > + >> > static int icl_color_check(struct intel_crtc_state *crtc_state) >> > { >> > int ret; >> > @@ -1614,6 +1628,8 @@ static int icl_color_check(struct >> intel_crtc_state *crtc_state) >> > >> > crtc_state->gamma_mode =3D icl_gamma_mode(crtc_state); >> > >> > + crtc_state->gamma_mode =3D dither_after_cc1_12bpc(crtc_state); >> > + >> > crtc_state->csc_mode =3D icl_csc_mode(crtc_state); >> > >> > crtc_state->preload_luts =3D intel_can_preload_luts(crtc_state); >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c >> b/drivers/gpu/drm/i915/display/intel_display.c >> > index 4dc4b1be0809..e3dbcd956fc6 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_display.c >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c >> > @@ -8098,9 +8098,15 @@ static void bdw_set_pipemisc(const struct >> intel_crtc_state *crtc_state) >> > break; >> > } >> > >> > - if (crtc_state->dither) >> > + /* >> > + * If 12bpc panel then, Enables dithering after the CC1 pipe >> > + * post color space conversion and not here >> > + */ >> > + >> > + if (crtc_state->dither && (crtc_state->pipe_bpp !=3D 36)) >> > val |=3D PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_S= P; >> > >> > + >> > if (crtc_state->output_format =3D=3D INTEL_OUTPUT_FORMAT_YCBCR42= 0 || >> > crtc_state->output_format =3D=3D INTEL_OUTPUT_FORMAT_YCBCR44= 4) >> > val |=3D PIPEMISC_OUTPUT_COLORSPACE_YUV; >> > @@ -10760,6 +10766,7 @@ intel_modeset_pipe_config(struct >> intel_atomic_state *state, >> > */ >> > Instead of... > > pipe_config->dither =3D (pipe_config->pipe_bpp =3D=3D 6*3) && >> > !pipe_config->dither_force_disable; >> > + >> > ... use ... > pipe_config->dither =3D ((pipe_config->pipe_bpp =3D=3D 6*3) || >> (HAS_DISPLAY13(i915) && pipe_config->pipe_bpp =3D=3D 12*3)) && >> !pipe_config->dither_force_disable; >> > ... so that the dither enable/disable decision and logging happens in one location in the code? > drm_dbg_kms(&i915->drm, >> > "hw max bpp: %i, pipe bpp: %i, dithering: %i\n", >> > base_bpp, pipe_config->pipe_bpp, pipe_config->dither= ); >> > Thanks, -mario > > diff --git a/drivers/gpu/drm/i915/i915_reg.h >> b/drivers/gpu/drm/i915/i915_reg.h >> > index 128b835c0adb..27f25214a839 100644 >> > --- a/drivers/gpu/drm/i915/i915_reg.h >> > +++ b/drivers/gpu/drm/i915/i915_reg.h >> > @@ -6132,7 +6132,7 @@ enum { >> > #define PIPEMISC_DITHER_8_BPC (0 << 5) >> > #define PIPEMISC_DITHER_10_BPC (1 << 5) >> > #define PIPEMISC_DITHER_6_BPC (2 << 5) >> > -#define PIPEMISC_DITHER_12_BPC (3 << 5) >> > +#define PIPEMISC_DITHER_12_BPC (4 << 5) >> > #define PIPEMISC_DITHER_ENABLE (1 << 4) >> > #define PIPEMISC_DITHER_TYPE_MASK (3 << 2) >> > #define PIPEMISC_DITHER_TYPE_SP (0 << 2) >> > @@ -7668,6 +7668,7 @@ enum { >> > #define GAMMA_MODE_MODE_12BIT (2 << 0) >> > #define GAMMA_MODE_MODE_SPLIT (3 << 0) /* ivb-bdw */ >> > #define GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED (3 << 0) /* icl = + >> */ >> > +#define GAMMA_MODE_DITHER_AFTER_CC1 (1 << 26) >> > >> > /* DMC/CSR */ >> > #define CSR_PROGRAM(i) _MMIO(0x80000 + (i) * 4) >> > -- >> > 2.25.4 >> > >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Ville Syrj=C3=A4l=C3=A4 >> Intel >> > --000000000000ed81e305bba9f2fc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Fri, Feb 19, 2021 at 4:22 AM Mario= Kleiner <mario.kleiner.de= @gmail.com> wrote:


On Thu, Feb 11,= 2021 at 1:29 PM Ville Syrj=C3=A4l=C3=A4 <ville.syrjala@linux.intel.com> = wrote:
On Thu, J= an 28, 2021 at 11:24:13AM -0800, Matt Roper wrote:
> From: Nischal Varide <nischal.varide@intel.com>
>
> If the panel is 12bpc then Dithering is not enabled in the Legacy
> dithering block , instead its Enabled after the C1 CC1 pipe post
> color space conversion.For a 6bpc pannel Dithering is enabled in
> Legacy block.

Dithering is probably going to require a whole uapi bikeshed.
Not sure we can just enable it unilaterally.

Ccing dri-devel, and Mario who had issues with dithering in the
past...

Thanks for the cc Ville!

The problem with dithering on Intel is that various tested Intel gpu'= s (Ironlake, IvyBridge, Haswell, Skylake iirc.) are dithering when they sho= uldn't. If one has a standard 8 bpc framebuffer feeding into a standard= (legacy) 256 slots, 8 bit wide lut which was loaded with an identity mappi= ng, feeding into a standard 8 bpc video output (DVI/HDMI/DP), the expected = result is that pixels rendered into the framebuffer show up unmodified at t= he video output. What happens instead is that some dithering is needlessly = applied. This is bad for various neuroscience/medical research equipment th= at requires pixels to pass unmodified in a pure 8 bpc configuration, e.g., = because some digital info is color-encoded in-band in the rendered image to= control research hardware, a la "if rgb pixel (123, 12, 23) is detect= ed in the digital video stream, emit some trigger signal, or timestamp that= moment with a hw clock, or start or stop some scientific recording equipme= nt". Also there exist specialized visual stimulators to drive special = displays with more than 12 bpc, e.g., 16 bpc, and so they encode the 8MSB o= f 16 bpc color values in pixels in even columns, and the 8LSB in the odd co= lumns of the framebuffer. Unexpected dithering makes such equipment complet= ely unusable. By now I must have spent months of my life, just trying to de= al with dithering induced problems on different gpu's due to hw quirks = or bugs somewhere in the graphics stack.

Atm. = the intel kms driver disables dithering for anything with >=3D 8 bpc as = a fix for this harmful hardware quirk.

Ideally we= 'd have uapi that makes dithering controllable per connector (on/off/au= to, selectable depth), also in a way that those controls are exposed as Ran= dR output properties, easily controllable by X clients. And some safe defau= lt in case the client can't access the properties (like I'd expect = to happen with the dozens of Wayland compositors under the sun). Various dr= ivers had this over time, e.g., AMD classic kms path (if i don't misrem= ember) and nouveau, but some of it also got lost in the new atomic kms vari= ants, and Intel never exposed this.

Or maybe some metho= d that checks the values actually stored in the hw lut's, CTM etc. and = if the values suggest no dithering should be needed, disable the dithering.= E.g., if output depth is 8 bpc, one only needs dithering if the slots in t= he final active hw lut do have any meaningful values in the lower bits belo= w the top 8 MSB, ie. if the content is actually > 8 bpc net bit depth.

-mario


One cup of coffee later... I think this specific patch= should be ok wrt. my use cases. The majority of the above mentioned resear= ch devices are single/dual-link DVI receivers, ie. 8 bpc video sinks. I'= ;m only aware of one recent device that has a DisplayPort receiver who coul= d act as a > 8 bpc video sink. See the following link for advanced examp= les of such devices: https://vpixx.com/our-products/video-i-o-hub/

<= div>I cannot think of a use case that would require more than 8 bits for in= band signalling given that that was good enough for the last 20 years, or f= or encoding very high color precision content -- the 16 bpc precision that = one can get out of the current even/odd pixel =3D 8 MSB + 8 LSB encoding sc= heme should be enough for the foreseeable future. Therefore dithering shoul= dn't pose a problem if it leaves the 8 MSB of each pixel color componen= t intact, and spatial dithering as employed here usually only touches the l= east significant bit (or maybe the 2 LSB's?).

= As this patch only enables dithering on 12 bpc video sinks, if i understand= pipe_bpp correctly, it could only "corrupt" one bit and leave at= least the 10-11 MSB's intact, right?

pipe_bpp= =3D=3D 24 is the case that would really hurt a lot of researchers if dithe= ring would be enabled without providing good uapi or other mechanisms to pr= event it.

So:

Acked-by: <= span style=3D"font-family:arial,sans-serif">Mario Kleiner <mario.kleiner.de@gmail.com>

One suggestion: It would be good to also add a = bit of drm_dbg_kms() logging to the new code-patch, so that this 12 bpc dit= hering enable on HAS_DISPLAY13 hw also shows up in the logs, not just the s= tandard 6 bpc enable. Helped a lot in debugging dithering issues if there w= as a reliable trace in the logs of what was active when. One suggestion for= that inside your patch below...

>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Nischal Varide <nischal.varide@intel.com>
> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>=C2=A0 drivers/gpu/drm/i915/display/intel_color.c=C2=A0 =C2=A0| 16 ++++= ++++++++++++
>=C2=A0 drivers/gpu/drm/i915/display/intel_display.c |=C2=A0 9 ++++++++-=
>=C2=A0 drivers/gpu/drm/i915/i915_reg.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 |=C2=A0 3 ++-
>=C2=A0 3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/= drm/i915/display/intel_color.c
> index ff7dcb7088bf..9a0572bbc5db 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1604,6 +1604,20 @@ static u32 icl_csc_mode(const struct intel_crtc= _state *crtc_state)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return csc_mode;
>=C2=A0 }
>=C2=A0
> +static u32 dither_after_cc1_12bpc(const struct intel_crtc_state *crtc= _state)
> +{
> +=C2=A0 =C2=A0 =C2=A0u32 gamma_mode =3D crtc_state->gamma_mode;
> +=C2=A0 =C2=A0 =C2=A0struct drm_i915_private *i915 =3D to_i915(crtc_st= ate->uapi.crtc->dev);
> +
> +=C2=A0 =C2=A0 =C2=A0if (HAS_DISPLAY13(i915)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!crtc_state->d= ither_force_disable &&

Replace=C2=A0 !crtc_state->dither_force_disable by crtc= _state->dither
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(crtc_s= tate->pipe_bpp =3D=3D 36))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0gamma_mode |=3D GAMMA_MODE_DITHER_AFTER_CC1;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0return gamma_mode;
> +}
> +
>=C2=A0 static int icl_color_check(struct intel_crtc_state *crtc_state)<= br> >=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0int ret;
> @@ -1614,6 +1628,8 @@ static int icl_color_check(struct intel_crtc_sta= te *crtc_state)
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0crtc_state->gamma_mode =3D icl_gamma_mode= (crtc_state);
>=C2=A0
> +=C2=A0 =C2=A0 =C2=A0crtc_state->gamma_mode =3D dither_after_cc1_12= bpc(crtc_state);
> +
>=C2=A0 =C2=A0 =C2=A0 =C2=A0crtc_state->csc_mode =3D icl_csc_mode(crt= c_state);
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0crtc_state->preload_luts =3D intel_can_pr= eload_luts(crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gp= u/drm/i915/display/intel_display.c
> index 4dc4b1be0809..e3dbcd956fc6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -8098,9 +8098,15 @@ static void bdw_set_pipemisc(const struct intel= _crtc_state *crtc_state)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0
> -=C2=A0 =C2=A0 =C2=A0if (crtc_state->dither)
> +=C2=A0 =C2=A0 =C2=A0/*
> +=C2=A0 =C2=A0 =C2=A0 * If 12bpc panel then, Enables dithering after t= he CC1 pipe
> +=C2=A0 =C2=A0 =C2=A0 * post color space conversion and not here
> +=C2=A0 =C2=A0 =C2=A0 */
> +
> +=C2=A0 =C2=A0 =C2=A0if (crtc_state->dither && (crtc_state-= >pipe_bpp !=3D 36))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0val |=3D PIPEMIS= C_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>=C2=A0
> +
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (crtc_state->output_format =3D=3D INTE= L_OUTPUT_FORMAT_YCBCR420 ||
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0crtc_state->output_format = =3D=3D INTEL_OUTPUT_FORMAT_YCBCR444)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0val |=3D PIPEMIS= C_OUTPUT_COLORSPACE_YUV;
> @@ -10760,6 +10766,7 @@ intel_modeset_pipe_config(struct intel_atomic_= state *state,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 */

Instead of...
=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0pipe_config->dither =3D (pipe_config->= pipe_bpp =3D=3D 6*3) &&
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0!pipe_config->= ;dither_force_disable;
> +

... use= ...

>=C2=A0 =C2=A0 =C2=A0 =C2=A0pipe_config->dither =3D ((pipe_config->= ;pipe_bpp =3D=3D 6*3) || (HAS_DISPLAY13(i915) && pipe_config->pi= pe_bpp =3D=3D 12*3)) && !pipe_config->dither_force_disable;

= ... so that the dither enable/disable decision and logging happens in one l= ocation in the code?

>=C2=A0 =C2=A0 =C2=A0 =C2=A0drm_dbg_kms(&i915->drm,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&q= uot;hw max bpp: %i, pipe bpp: %i, dithering: %i\n",
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ba= se_bpp, pipe_config->pipe_bpp, pipe_config->dither);
=

Thanks,
-mario


=C2=A0
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i9= 15_reg.h
> index 128b835c0adb..27f25214a839 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6132,7 +6132,7 @@ enum {
>=C2=A0 #define=C2=A0 =C2=A0PIPEMISC_DITHER_8_BPC=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 (0 << 5)
>=C2=A0 #define=C2=A0 =C2=A0PIPEMISC_DITHER_10_BPC=C2=A0 =C2=A0 =C2=A0(1= << 5)
>=C2=A0 #define=C2=A0 =C2=A0PIPEMISC_DITHER_6_BPC=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 (2 << 5)
> -#define=C2=A0 =C2=A0PIPEMISC_DITHER_12_BPC=C2=A0 =C2=A0 =C2=A0(3 <= < 5)
> +#define=C2=A0 =C2=A0PIPEMISC_DITHER_12_BPC=C2=A0 =C2=A0 =C2=A0(4 <= < 5)
>=C2=A0 #define=C2=A0 =C2=A0PIPEMISC_DITHER_ENABLE=C2=A0 =C2=A0 =C2=A0(1= << 4)
>=C2=A0 #define=C2=A0 =C2=A0PIPEMISC_DITHER_TYPE_MASK=C2=A0 (3 << = 2)
>=C2=A0 #define=C2=A0 =C2=A0PIPEMISC_DITHER_TYPE_SP=C2=A0 =C2=A0 (0 <= < 2)
> @@ -7668,6 +7668,7 @@ enum {
>=C2=A0 #define=C2=A0 GAMMA_MODE_MODE_12BIT=C2=A0 =C2=A0 =C2=A0 =C2=A0(2= << 0)
>=C2=A0 #define=C2=A0 GAMMA_MODE_MODE_SPLIT=C2=A0 =C2=A0 =C2=A0 =C2=A0(3= << 0) /* ivb-bdw */
>=C2=A0 #define=C2=A0 GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED=C2=A0 =C2=A0= =C2=A0 =C2=A0(3 << 0) /* icl + */
> +#define=C2=A0 GAMMA_MODE_DITHER_AFTER_CC1 (1 << 26)
>=C2=A0
>=C2=A0 /* DMC/CSR */
>=C2=A0 #define CSR_PROGRAM(i)=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0_MMIO(0x80000 + (i) * 4)
> --
> 2.25.4
>
> _______________________________________________
> Intel-gfx mailing list
> I= ntel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/l= istinfo/intel-gfx

--
Ville Syrj=C3=A4l=C3=A4
Intel
--000000000000ed81e305bba9f2fc-- --===============0196590124== 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 https://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0196590124==--