date:20210430


On April 30, 2021 18:00:58 "Dixit, Ashutosh"  wrote:


On Fri, 30 Apr 2021 15:26:09 -0700, Umesh Nerlige Ramappa wrote:


Looks like the engine can be dropped since all timestamps are in sync. I
just have one more question here. The timestamp itself is 36 bits.  Should
the uapi also report the timestamp width to the user OR should I just
return the lower 32 bits of the timestamp?


Yeah, I think reporting the timestamp width is a good idea since we're 
reporting the period/frequency here.





How would exposing only the lower 32 bits of the timestamp work?

The way to avoid exposing the width would be to expose the timestamp as a
regular 64 bit value. In the kernel engine state, have a variable for the
counter and keep on accumulating that (on each query) to full 64 bits in
spite of the 36 bit HW counter overflow.


That's doesn't actually work since you can query the 64-bit timestamp value 
from the GPU. The way this is handled in Vulkan is that the number of 
timestamp bits is reported to the application as a queue property.


--Jason





___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/tgl+: Add the missing MC CCS/XYUV8888 format support

2021-04-30 Thread Imre Deak

Make sure that the XYUV format is handled correctly when it's used
with a MC_CCS modifier framebuffer. Besides this format not working, the
driver will also return an incorrect error value when trying to use it,
indicating that the second color plane in the framebuffer is set
unexpectedly.

Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/display/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 6280ba7f4c172..13ed9055918fe 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1263,6 +1263,9 @@ static const struct drm_format_info gen12_ccs_formats[] = 
{
{ .format = DRM_FORMAT_VYUY, .num_planes = 2,
  .char_per_block = { 2, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
  .hsub = 2, .vsub = 1, .is_yuv = true },
+   { .format = DRM_FORMAT_XYUV, .num_planes = 2,
+ .char_per_block = { 4, 1 }, .block_w = { 1, 2 }, .block_h = { 1, 1 },
+ .hsub = 1, .vsub = 1, .is_yuv = true },
{ .format = DRM_FORMAT_NV12, .num_planes = 4,
  .char_per_block = { 1, 2, 1, 1 }, .block_w = { 1, 1, 4, 4 }, .block_h 
= { 1, 1, 1, 1 },
  .hsub = 2, .vsub = 2, .is_yuv = true },
-- 
2.27.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/1] i915/query: Correlate engine and cpu timestamps with better accuracy

2021-04-30 Thread Dixit, Ashutosh

On Fri, 30 Apr 2021 16:00:46 -0700, Dixit, Ashutosh wrote:
>
> On Fri, 30 Apr 2021 15:26:09 -0700, Umesh Nerlige Ramappa wrote:
> >
> > Looks like the engine can be dropped since all timestamps are in sync. I
> > just have one more question here. The timestamp itself is 36 bits.  Should
> > the uapi also report the timestamp width to the user OR should I just
> > return the lower 32 bits of the timestamp?
>
> How would exposing only the lower 32 bits of the timestamp work?

It would work I guess but overflow every few seconds. So if the counters
are sampled at a low frequency (once every few seconds) it would yield
misleading timestamps.

> The way to avoid exposing the width would be to expose the timestamp as a
> regular 64 bit value. In the kernel engine state, have a variable for the
> counter and keep on accumulating that (on each query) to full 64 bits in
> spite of the 36 bit HW counter overflow.
>
> So not exposing the width (or exposing a 64 bit timestamp) is a cleaner
> interface but also more work in the kernel.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/1] i915/query: Correlate engine and cpu timestamps with better accuracy

2021-04-30 Thread Dixit, Ashutosh

On Fri, 30 Apr 2021 15:26:09 -0700, Umesh Nerlige Ramappa wrote:
>
> Looks like the engine can be dropped since all timestamps are in sync. I
> just have one more question here. The timestamp itself is 36 bits.  Should
> the uapi also report the timestamp width to the user OR should I just
> return the lower 32 bits of the timestamp?

How would exposing only the lower 32 bits of the timestamp work?

The way to avoid exposing the width would be to expose the timestamp as a
regular 64 bit value. In the kernel engine state, have a variable for the
counter and keep on accumulating that (on each query) to full 64 bits in
spite of the 36 bit HW counter overflow.

So not exposing the width (or exposing a 64 bit timestamp) is a cleaner
interface but also more work in the kernel.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Use the correct max source link rate for MST

2021-04-30 Thread Cornij, Nikola

[AMD Official Use Only - Internal Distribution Only]

I'll fix the dpcd part to use kHz on Monday

My apologies as well, not only for coming up with the wrong patch in first 
place, but also for missing to CC all the maintainers.

-Original Message-
From: Lyude Paul 
Sent: Friday, April 30, 2021 6:41 PM
To: Cornij, Nikola ; amd-...@lists.freedesktop.org
Cc: Pillai, Aurabindo ; Lipski, Mikita 
; ville.syrj...@linux.intel.com; koba...@canonical.com; 
intel-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; 
jani.nik...@linux.intel.com; bske...@redhat.com
Subject: Re: [PATCH v2 1/1] drm/i915: Use the correct max source link rate for 
MST

Alright - I had Ville double check this and give their A-B on IRC (I just need 
to fix the open coded link_rate_to_bw() here). Since this got broken on drm- 
misc-next I'm going to go ahead and push the fix there, since I'm not going to 
be around next Monday or Tuesday and I don't want to leave i915 broken in the 
interim. Sorry about the noise with this Jani!

As well - I'll get to fixing the dpcd unit usage once I get back on Wednesday 
(unless Nikola's already gotten to it by then) so we use KHz instead. Although 
as ville has pointed out, I think we should teach the topology manager to allow 
passing for the current link rate/lane count during state computation in the 
long term.

On Fri, 2021-04-30 at 17:45 -0400, Nikola Cornij wrote:
> [why]
> Previously used value was not safe to provide the correct value, i.e.
> it could be 0 if not not configured, leading to no MST on this platform.
>
> [how]
> Do not use the value from BIOS, but from the structure populated at
> encoder initialization time.
>
> Fixes: 98025a62cb00 ("drm/dp_mst: Use Extended Base Receiver
> Capability DPCD
> space")
> Signed-off-by: Nikola Cornij 
> Reviewed-by: Lyude Paul 
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index bf7f8487945c..3642d7578658 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -942,7 +942,7 @@ intel_dp_mst_encoder_init(struct
> intel_digital_port *dig_port, int conn_base_id)
> struct intel_dp *intel_dp = _port->dp;
> enum port port = dig_port->base.port;
> int ret;
> -   int bios_max_link_rate;
> +   int max_source_rate = intel_dp->source_rates[intel_dp-
> >num_source_rates - 1];
>
> if (!HAS_DP_MST(i915) || intel_dp_is_edp(intel_dp))
> return 0;
> @@ -957,11 +957,11 @@ intel_dp_mst_encoder_init(struct
> intel_digital_port *dig_port, int conn_base_id)
>
> /* create encoders */
> intel_dp_create_fake_mst_encoders(dig_port);
> -   bios_max_link_rate =
> intel_bios_dp_max_link_rate(_port->base);
> ret = drm_dp_mst_topology_mgr_init(_dp->mst_mgr,
> >drm,
>_dp->aux, 16, 3,
>(u8)dig_port->max_lanes,
> -  (u8)(bios_max_link_rate /
> 27000), conn_base_id);
> +  (u8)(max_source_rate /
> +27000),
> +  conn_base_id);
> if (ret)
> return ret;
>

--
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat

Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've 
asked me a question, are waiting for a review/merge on a patch, etc. and I 
haven't responded in a while, please feel free to send me another email to 
check on my status. I don't bite!

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/1] drm/i915: Use the correct max source link rate for MST

Alright - I had Ville double check this and give their A-B on IRC (I just need
to fix the open coded link_rate_to_bw() here). Since this got broken on drm-
misc-next I'm going to go ahead and push the fix there, since I'm not going to
be around next Monday or Tuesday and I don't want to leave i915 broken in the
interim. Sorry about the noise with this Jani!

As well - I'll get to fixing the dpcd unit usage once I get back on Wednesday
(unless Nikola's already gotten to it by then) so we use KHz instead. Although
as ville has pointed out, I think we should teach the topology manager to allow
passing for the current link rate/lane count during state computation in the
long term.

On Fri, 2021-04-30 at 17:45 -0400, Nikola Cornij wrote:
> [why]
> Previously used value was not safe to provide the correct value, i.e. it
> could be 0 if not not configured, leading to no MST on this platform.
> 
> [how]
> Do not use the value from BIOS, but from the structure populated at
> encoder initialization time.
> 
> Fixes: 98025a62cb00 ("drm/dp_mst: Use Extended Base Receiver Capability DPCD
> space")
> Signed-off-by: Nikola Cornij 
> Reviewed-by: Lyude Paul 
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index bf7f8487945c..3642d7578658 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -942,7 +942,7 @@ intel_dp_mst_encoder_init(struct intel_digital_port
> *dig_port, int conn_base_id)
> struct intel_dp *intel_dp = _port->dp;
> enum port port = dig_port->base.port;
> int ret;
> -   int bios_max_link_rate;
> +   int max_source_rate = intel_dp->source_rates[intel_dp-
> >num_source_rates - 1];
>  
> if (!HAS_DP_MST(i915) || intel_dp_is_edp(intel_dp))
> return 0;
> @@ -957,11 +957,11 @@ intel_dp_mst_encoder_init(struct intel_digital_port
> *dig_port, int conn_base_id)
>  
> /* create encoders */
> intel_dp_create_fake_mst_encoders(dig_port);
> -   bios_max_link_rate = intel_bios_dp_max_link_rate(_port->base);
> ret = drm_dp_mst_topology_mgr_init(_dp->mst_mgr, >drm,
>    _dp->aux, 16, 3,
>    (u8)dig_port->max_lanes,
> -  (u8)(bios_max_link_rate / 27000),
> conn_base_id);
> +  (u8)(max_source_rate / 27000),
> +  conn_base_id);
> if (ret)
> return ret;
>  

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat

Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [CI 2/4] drm/i915/display: remove FIXME comment for intended feature

Direction on gen >= 9 was to stop using straps and rely on VBT
indicating if the port is present or not. Remove FIXME comment since
this will never be "fixed".

Signed-off-by: Lucas De Marchi 
Reviewed-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_display.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 705f32b2f832..37fe35f6de2c 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10879,15 +10879,9 @@ static void intel_setup_outputs(struct 
drm_i915_private *dev_priv)
 
icl_dsi_init(dev_priv);
} else if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) {
-   /*
-* FIXME: Broxton doesn't support port detection via the
-* DDI_BUF_CTL_A or SFUSE_STRAP registers, find another way to
-* detect the ports.
-*/
intel_ddi_init(dev_priv, PORT_A);
intel_ddi_init(dev_priv, PORT_B);
intel_ddi_init(dev_priv, PORT_C);
-
vlv_dsi_init(dev_priv);
} else if (HAS_DDI(dev_priv)) {
int found;
-- 
2.31.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [CI 4/4] drm/i915/display: hide workaround for broken vbt in intel_bios.c

Instead of poluting the normal code path in intel_display.c, make
intel_bios.c handle the brokenness of the VBT.

Signed-off-by: Lucas De Marchi 
Reviewed-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_bios.c| 20 
 drivers/gpu/drm/i915/display/intel_display.c | 14 ++
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
b/drivers/gpu/drm/i915/display/intel_bios.c
index befab891a6b9..a783c5a40934 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1852,6 +1852,19 @@ intel_bios_encoder_supports_edp(const struct 
intel_bios_encoder_data *devdata)
devdata->child.device_type & DEVICE_TYPE_INTERNAL_CONNECTOR;
 }
 
+static bool is_port_valid(struct drm_i915_private *i915, enum port port)
+{
+   /*
+* On some ICL/CNL SKUs port F is not present, but broken VBTs mark
+* the port as present. Only try to initialize port F for the
+* SKUs that may actually have it.
+*/
+   if (port == PORT_F && (IS_ICELAKE(i915) || IS_CANNONLAKE(i915)))
+   return IS_ICL_WITH_PORT_F(i915) || IS_CNL_WITH_PORT_F(i915);
+
+   return true;
+}
+
 static void parse_ddi_port(struct drm_i915_private *i915,
   struct intel_bios_encoder_data *devdata)
 {
@@ -1865,6 +1878,13 @@ static void parse_ddi_port(struct drm_i915_private *i915,
if (port == PORT_NONE)
return;
 
+   if (!is_port_valid(i915, port)) {
+   drm_dbg_kms(>drm,
+   "VBT reports port %c as supported, but that can't 
be true: skipping\n",
+   port_name(port));
+   return;
+   }
+
info = >vbt.ddi_port_info[port];
 
if (info->devdata) {
diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 55f8f2ceada2..87a85de5e03d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10868,15 +10868,7 @@ static void intel_setup_outputs(struct 
drm_i915_private *dev_priv)
intel_ddi_init(dev_priv, PORT_C);
intel_ddi_init(dev_priv, PORT_D);
intel_ddi_init(dev_priv, PORT_E);
-
-   /*
-* On some ICL SKUs port F is not present, but broken VBTs mark
-* the port as present. Only try to initialize port F for the
-* SKUs that may actually have it.
-*/
-   if (IS_ICL_WITH_PORT_F(dev_priv))
-   intel_ddi_init(dev_priv, PORT_F);
-
+   intel_ddi_init(dev_priv, PORT_F);
icl_dsi_init(dev_priv);
} else if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) {
intel_ddi_init(dev_priv, PORT_A);
@@ -10889,9 +10881,7 @@ static void intel_setup_outputs(struct drm_i915_private 
*dev_priv)
intel_ddi_init(dev_priv, PORT_C);
intel_ddi_init(dev_priv, PORT_D);
intel_ddi_init(dev_priv, PORT_E);
-
-   if (IS_CNL_WITH_PORT_F(dev_priv))
-   intel_ddi_init(dev_priv, PORT_F);
+   intel_ddi_init(dev_priv, PORT_F);
} else if (HAS_DDI(dev_priv)) {
u32 found;
 
-- 
2.31.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [CI 3/4] drm/i915/display: remove strap checks from gen 9

Direction on gen9+ was to stop reading the straps and only rely on the
VBT for marking the port presence. This happened while dealing with
WaIgnoreDDIAStrap and instead of using it as a WA, it should now be the
normal flow. See commit 885d3e5b6f08 ("drm/i915/display: fix comment on
skl straps").

For gen 10 it's hard to say if this will work or not since I can't test
it, so leave it with the same behavior as before.

For PCH_TGP we should still rely on the VBT to make ports E and F not
available.

v2 (Ville):
  - use display ver >= 9 to make it consistent with the rest of the
driver instead of checking for == 9
  - also handle CNL and only initialize port F if it is
IS_CNL_WITH_PORT_F. Eventually CNL may be removed, but while it
isn't let's keep it consistent everywhere

Signed-off-by: Lucas De Marchi 
Reviewed-by: Anusha Srivatsa 
Reviewed-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_display.c | 38 +++-
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 37fe35f6de2c..55f8f2ceada2 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10883,34 +10883,27 @@ static void intel_setup_outputs(struct 
drm_i915_private *dev_priv)
intel_ddi_init(dev_priv, PORT_B);
intel_ddi_init(dev_priv, PORT_C);
vlv_dsi_init(dev_priv);
+   } else if (DISPLAY_VER(dev_priv) >= 9) {
+   intel_ddi_init(dev_priv, PORT_A);
+   intel_ddi_init(dev_priv, PORT_B);
+   intel_ddi_init(dev_priv, PORT_C);
+   intel_ddi_init(dev_priv, PORT_D);
+   intel_ddi_init(dev_priv, PORT_E);
+
+   if (IS_CNL_WITH_PORT_F(dev_priv))
+   intel_ddi_init(dev_priv, PORT_F);
} else if (HAS_DDI(dev_priv)) {
-   int found;
+   u32 found;
 
if (intel_ddi_crt_present(dev_priv))
intel_crt_init(dev_priv);
 
-   /*
-* Haswell uses DDI functions to detect digital outputs.
-* On SKL pre-D0 the strap isn't connected. Later SKUs may or
-* may not have it - it was supposed to be fixed by the same
-* time we stopped using straps. Assume it's there.
-*/
+   /* Haswell uses DDI functions to detect digital outputs. */
found = intel_de_read(dev_priv, DDI_BUF_CTL(PORT_A)) & 
DDI_INIT_DISPLAY_DETECTED;
-   /* WaIgnoreDDIAStrap: skl */
-   if (found || DISPLAY_VER(dev_priv) == 9)
+   if (found)
intel_ddi_init(dev_priv, PORT_A);
 
-   /* DDI B, C, D, and F detection is indicated by the SFUSE_STRAP
-* register */
-   if (HAS_PCH_TGP(dev_priv)) {
-   /* W/A due to lack of STRAP config on TGP PCH*/
-   found = (SFUSE_STRAP_DDIB_DETECTED |
-SFUSE_STRAP_DDIC_DETECTED |
-SFUSE_STRAP_DDID_DETECTED);
-   } else {
-   found = intel_de_read(dev_priv, SFUSE_STRAP);
-   }
-
+   found = intel_de_read(dev_priv, SFUSE_STRAP);
if (found & SFUSE_STRAP_DDIB_DETECTED)
intel_ddi_init(dev_priv, PORT_B);
if (found & SFUSE_STRAP_DDIC_DETECTED)
@@ -10919,11 +10912,6 @@ static void intel_setup_outputs(struct 
drm_i915_private *dev_priv)
intel_ddi_init(dev_priv, PORT_D);
if (found & SFUSE_STRAP_DDIF_DETECTED)
intel_ddi_init(dev_priv, PORT_F);
-   /*
-* On SKL we don't have a way to detect DDI-E so we rely on VBT.
-*/
-   if (DISPLAY_VER(dev_priv) == 9)
-   intel_ddi_init(dev_priv, PORT_E);
} else if (HAS_PCH_SPLIT(dev_priv)) {
int found;
 
-- 
2.31.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [CI 0/4] Simplify intel_setup_outputs

First and second patches should be straightforward. Third patch is a
simplification for gen9+ since we are not supposed to check the straps
anymore and rely on VBT.

Last patch hides port F hacks in intel_bios.c so we have a clean init
sequence.

Lucas De Marchi (4):
  drm/i915/display: move vbt check to intel_ddi_init()
  drm/i915/display: remove FIXME comment for intended feature
  drm/i915/display: remove strap checks from gen 9
  drm/i915/display: hide workaround for broken vbt in intel_bios.c

 drivers/gpu/drm/i915/display/intel_bios.c| 20 
 drivers/gpu/drm/i915/display/intel_display.c | 54 +---
 2 files changed, 32 insertions(+), 42 deletions(-)

-- 
2.31.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [CI 1/4] drm/i915/display: move vbt check to intel_ddi_init()

Since commit 45c0673aac97 ("drm/i915/bios: start using the
intel_bios_encoder_data directly") we lookup the devdata for each port
in intel_ddi_init() and just return if the port is not present in VBT
(or if we didn't create a fake devdata for it if VBT is not available).

So in intel_display.c we don't have to check
intel_bios_is_port_present(), just rely on the check in
intel_ddi_init().

v2: Rebase on commit 45c0673aac97 ("drm/i915/bios: start using the
intel_bios_encoder_data directly") re-using that check in intel_ddi_init()
instead of adding a new one.

Signed-off-by: Lucas De Marchi 
Reviewed-by: Jani Nikula 
---
 drivers/gpu/drm/i915/display/intel_display.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 6280ba7f4c17..705f32b2f832 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -10868,13 +10868,13 @@ static void intel_setup_outputs(struct 
drm_i915_private *dev_priv)
intel_ddi_init(dev_priv, PORT_C);
intel_ddi_init(dev_priv, PORT_D);
intel_ddi_init(dev_priv, PORT_E);
+
/*
-* On some ICL SKUs port F is not present. No strap bits for
-* this, so rely on VBT.
-* Work around broken VBTs on SKUs known to have no port F.
+* On some ICL SKUs port F is not present, but broken VBTs mark
+* the port as present. Only try to initialize port F for the
+* SKUs that may actually have it.
 */
-   if (IS_ICL_WITH_PORT_F(dev_priv) &&
-   intel_bios_is_port_present(dev_priv, PORT_F))
+   if (IS_ICL_WITH_PORT_F(dev_priv))
intel_ddi_init(dev_priv, PORT_F);
 
icl_dsi_init(dev_priv);
@@ -10928,10 +10928,8 @@ static void intel_setup_outputs(struct 
drm_i915_private *dev_priv)
/*
 * On SKL we don't have a way to detect DDI-E so we rely on VBT.
 */
-   if (DISPLAY_VER(dev_priv) == 9 &&
-   intel_bios_is_port_present(dev_priv, PORT_E))
+   if (DISPLAY_VER(dev_priv) == 9)
intel_ddi_init(dev_priv, PORT_E);
-
} else if (HAS_PCH_SPLIT(dev_priv)) {
int found;
 
-- 
2.31.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/dp: Drop open-coded drm_dp_is_branch() in drm_dp_read_downstream_info()

Noticed this while fixing another issue in drm_dp_read_downstream_info(),
the open coded DP_DOWNSTREAMPORT_PRESENT check here just duplicates what we
already do in drm_dp_is_branch(), so just get rid of it.

Signed-off-by: Lyude Paul 
---
 drivers/gpu/drm/drm_dp_helper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 27c8c5bdf7d9..0f84df8798ab 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -677,9 +677,7 @@ int drm_dp_read_downstream_info(struct drm_dp_aux *aux,
memset(downstream_ports, 0, DP_MAX_DOWNSTREAM_PORTS);
 
/* No downstream info to read */
-   if (!drm_dp_is_branch(dpcd) ||
-   dpcd[DP_DPCD_REV] < DP_DPCD_REV_10 ||
-   !(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
+   if (!drm_dp_is_branch(dpcd) || dpcd[DP_DPCD_REV] < DP_DPCD_REV_10)
return 0;
 
/* Some branches advertise having 0 downstream ports, despite also 
advertising they have a
-- 
2.30.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/2] drm/dp: Handle zeroed port counts in drm_dp_read_downstream_info()

While the DP specification isn't entirely clear on if this should be
allowed or not, some branch devices report having downstream ports present
while also reporting a downstream port count of 0. So to avoid breaking
those devices, we need to handle this in drm_dp_read_downstream_info().

So, to do this we assume there's no downstream port info when the
downstream port count is 0.

Signed-off-by: Lyude Paul 
Tested-by: Jérôme de Bretagne 
Bugzilla: https://gitlab.freedesktop.org/drm/intel/-/issues/3416
Fixes: 3d3721ccb18a ("drm/i915/dp: Extract drm_dp_read_downstream_info()")
Cc:  # v5.10+
---
 drivers/gpu/drm/drm_dp_helper.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index cb56d74e9d38..27c8c5bdf7d9 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -682,7 +682,14 @@ int drm_dp_read_downstream_info(struct drm_dp_aux *aux,
!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
return 0;
 
+   /* Some branches advertise having 0 downstream ports, despite also 
advertising they have a
+* downstream port present. The DP spec isn't clear on if this is 
allowed or not, but since
+* some branches do it we need to handle it regardless.
+*/
len = drm_dp_downstream_port_count(dpcd);
+   if (!len)
+   return 0;
+
if (dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DETAILED_CAP_INFO_AVAILABLE)
len *= 4;
 
-- 
2.30.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/1] i915/query: Correlate engine and cpu timestamps with better accuracy

2021-04-30 Thread Umesh Nerlige Ramappa


On Thu, Apr 29, 2021 at 02:07:58PM -0500, Jason Ekstrand wrote:

On Wed, Apr 28, 2021 at 7:34 PM Umesh Nerlige Ramappa
 wrote:


Perf measurements rely on CPU and engine timestamps to correlate
events of interest across these time domains. Current mechanisms get
these timestamps separately and the calculated delta between these
timestamps lack enough accuracy.

To improve the accuracy of these time measurements to within a few us,
add a query that returns the engine and cpu timestamps captured as
close to each other as possible.

v2: (Tvrtko)
- document clock reference used
- return cpu timestamp always
- capture cpu time just before lower dword of cs timestamp

v3: (Chris)
- use uncore-rpm
- use __query_cs_timestamp helper

v4: (Lionel)
- Kernel perf subsytem allows users to specify the clock id to be used
  in perf_event_open. This clock id is used by the perf subsystem to
  return the appropriate cpu timestamp in perf events. Similarly, let
  the user pass the clockid to this query so that cpu timestamp
  corresponds to the clock id requested.

v5: (Tvrtko)
- Use normal ktime accessors instead of fast versions
- Add more uApi documentation

v6: (Lionel)
- Move switch out of spinlock

v7: (Chris)
- cs_timestamp is a misnomer, use cs_cycles instead
- return the cs cycle frequency as well in the query

v8:
- Add platform and engine specific checks

v9: (Lionel)
- Return 2 cpu timestamps in the query - captured before and after the
  register read

v10: (Chris)
- Use local_clock() to measure time taken to read lower dword of
  register and return it to user.

v11: (Jani)
- IS_GEN deprecated. User GRAPHICS_VER instead.

v12: (Jason)
- Split cpu timestamp array into timestamp and delta for cleaner API

Signed-off-by: Umesh Nerlige Ramappa 
Reviewed-by: Lionel Landwerlin 
---
 drivers/gpu/drm/i915/i915_query.c | 148 ++
 include/uapi/drm/i915_drm.h   |  52 +++
 2 files changed, 200 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c 
b/drivers/gpu/drm/i915/i915_query.c
index fed337ad7b68..357c44e8177c 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -6,6 +6,8 @@

 #include 

+#include "gt/intel_engine_pm.h"
+#include "gt/intel_engine_user.h"
 #include "i915_drv.h"
 #include "i915_perf.h"
 #include "i915_query.h"
@@ -90,6 +92,151 @@ static int query_topology_info(struct drm_i915_private 
*dev_priv,
return total_length;
 }

+typedef u64 (*__ktime_func_t)(void);
+static __ktime_func_t __clock_id_to_func(clockid_t clk_id)
+{
+   /*
+* Use logic same as the perf subsystem to allow user to select the
+* reference clock id to be used for timestamps.
+*/
+   switch (clk_id) {
+   case CLOCK_MONOTONIC:
+   return _get_ns;
+   case CLOCK_MONOTONIC_RAW:
+   return _get_raw_ns;
+   case CLOCK_REALTIME:
+   return _get_real_ns;
+   case CLOCK_BOOTTIME:
+   return _get_boottime_ns;
+   case CLOCK_TAI:
+   return _get_clocktai_ns;
+   default:
+   return NULL;
+   }
+}
+
+static inline int
+__read_timestamps(struct intel_uncore *uncore,
+ i915_reg_t lower_reg,
+ i915_reg_t upper_reg,
+ u64 *cs_ts,
+ u64 *cpu_ts,
+ u64 *cpu_delta,
+ __ktime_func_t cpu_clock)
+{
+   u32 upper, lower, old_upper, loop = 0;
+
+   upper = intel_uncore_read_fw(uncore, upper_reg);
+   do {
+   *cpu_delta = local_clock();
+   *cpu_ts = cpu_clock();
+   lower = intel_uncore_read_fw(uncore, lower_reg);
+   *cpu_delta = local_clock() - *cpu_delta;
+   old_upper = upper;
+   upper = intel_uncore_read_fw(uncore, upper_reg);
+   } while (upper != old_upper && loop++ < 2);
+
+   *cs_ts = (u64)upper << 32 | lower;
+
+   return 0;
+}
+
+static int
+__query_cs_cycles(struct intel_engine_cs *engine,
+ u64 *cs_ts, u64 *cpu_ts, u64 *cpu_delta,
+ __ktime_func_t cpu_clock)
+{
+   struct intel_uncore *uncore = engine->uncore;
+   enum forcewake_domains fw_domains;
+   u32 base = engine->mmio_base;
+   intel_wakeref_t wakeref;
+   int ret;
+
+   fw_domains = intel_uncore_forcewake_for_reg(uncore,
+   RING_TIMESTAMP(base),
+   FW_REG_READ);
+
+   with_intel_runtime_pm(uncore->rpm, wakeref) {
+   spin_lock_irq(>lock);
+   intel_uncore_forcewake_get__locked(uncore, fw_domains);
+
+   ret = __read_timestamps(uncore,
+   RING_TIMESTAMP(base),
+   RING_TIMESTAMP_UDW(base),
+   cs_ts,
+   cpu_ts,
+

[Intel-gfx] [PATCH v2 1/1] drm/i915: Use the correct max source link rate for MST

[why]
Previously used value was not safe to provide the correct value, i.e. it
could be 0 if not not configured, leading to no MST on this platform.

[how]
Do not use the value from BIOS, but from the structure populated at
encoder initialization time.

Fixes: 98025a62cb00 ("drm/dp_mst: Use Extended Base Receiver Capability DPCD 
space")
Signed-off-by: Nikola Cornij 
Reviewed-by: Lyude Paul 
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c 
b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index bf7f8487945c..3642d7578658 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -942,7 +942,7 @@ intel_dp_mst_encoder_init(struct intel_digital_port 
*dig_port, int conn_base_id)
struct intel_dp *intel_dp = _port->dp;
enum port port = dig_port->base.port;
int ret;
-   int bios_max_link_rate;
+   int max_source_rate = intel_dp->source_rates[intel_dp->num_source_rates 
- 1];
 
if (!HAS_DP_MST(i915) || intel_dp_is_edp(intel_dp))
return 0;
@@ -957,11 +957,11 @@ intel_dp_mst_encoder_init(struct intel_digital_port 
*dig_port, int conn_base_id)
 
/* create encoders */
intel_dp_create_fake_mst_encoders(dig_port);
-   bios_max_link_rate = intel_bios_dp_max_link_rate(_port->base);
ret = drm_dp_mst_topology_mgr_init(_dp->mst_mgr, >drm,
   _dp->aux, 16, 3,
   (u8)dig_port->max_lanes,
-  (u8)(bios_max_link_rate / 27000), 
conn_base_id);
+  (u8)(max_source_rate / 27000),
+  conn_base_id);
if (ret)
return ret;
 
-- 
2.25.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 0/1] drm/i915: Use the correct max source link rate for MST

This is a follow-up change to fix incorrectly used max link rate source 
capability at MST init time.

Change history:

v1:
  - Initial

v2:
  - Use local variabale for improved code readability
  - Fix the comment to point to the correct sub-directory

Nikola Cornij (1):
  drm/i915: Use the correct max source link rate for MST

 drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.25.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v1 1/1] drm/dp_mst: Use the correct max source link rate for i915

On Fri, 2021-04-30 at 17:22 -0400, Nikola Cornij wrote:
> [why]
> Previously used value was not safe to provide the correct value, i.e. it
> could be 0 if not not configured, leading to no MST on this platform.
> 
> [how]
> Do not use the value from BIOS, but from the structure populated at
> encoder initialization time.
> 
> Fixes: 98025a62cb00 ("drm/dp_mst: Use Extended Base Receiver Capability DPCD
> space")
> Signed-off-by: Nikola Cornij 
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index bf7f8487945c..01a0ed99988f 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -942,7 +942,6 @@ intel_dp_mst_encoder_init(struct intel_digital_port
> *dig_port, int conn_base_id)
> struct intel_dp *intel_dp = _port->dp;
> enum port port = dig_port->base.port;
> int ret;
> -   int bios_max_link_rate;
>  
> if (!HAS_DP_MST(i915) || intel_dp_is_edp(intel_dp))
> return 0;
> @@ -957,11 +956,11 @@ intel_dp_mst_encoder_init(struct intel_digital_port
> *dig_port, int conn_base_id)
>  
> /* create encoders */
> intel_dp_create_fake_mst_encoders(dig_port);
> -   bios_max_link_rate = intel_bios_dp_max_link_rate(_port->base);
> ret = drm_dp_mst_topology_mgr_init(_dp->mst_mgr, >drm,
>    _dp->aux, 16, 3,
>    (u8)dig_port->max_lanes,
> -  (u8)(bios_max_link_rate / 27000),
> conn_base_id);
> +  (u8)(intel_dp-
> >source_rates[intel_dp->num_source_rates - 1] / 27000),
> +  conn_base_id);

This line is kind of long, I'd say we should just store the max link rate in a
local variable like max_link_rate, then just pass that to
drm_dp_mst_topology_mgr_init()

Also, the commit message should probably be:

drm/i915: Use the correct max source link rate for MST

With those two things fixed:

Reviewed-by: Lyude Paul 

> if (ret)
> return ret;
>  

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/display/tgl+: Add new sequence step for MST + FEC

2021-04-30 Thread José Roberto de Souza

For DP 1.4 sinks + MST + FEC it is required to prevent a FEC stall
signaling.

BSpec: 49190
BSpec: 54128
Signed-off-by: José Roberto de Souza 
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 28 +
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c 
b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 9e406d9722c5..e7b636ba6982 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -376,6 +376,30 @@ static void intel_mst_disable_dp(struct intel_atomic_state 
*state,
  old_crtc_state, old_conn_state);
 }
 
+static void intel_mst_config_fec(struct intel_encoder *encoder,
+const struct intel_crtc_state *crtc_state)
+{
+   struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+   struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+
+   if (DISPLAY_VER(i915) >= 12 && crtc_state->fec_enable &&
+   intel_dp->dpcd[DP_DPCD_REV] == DP_DPCD_REV_14)
+   intel_de_rmw(i915, CHICKEN_TRANS(crtc_state->cpu_transcoder),
+0, PREVENT_FEC_STALL_SIGNALING);
+}
+
+static void intel_mst_unconfig_fec(struct intel_encoder *encoder,
+  const struct intel_crtc_state *crtc_state)
+{
+   struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+   struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+
+   if (DISPLAY_VER(i915) >= 12 && crtc_state->fec_enable &&
+   intel_dp->dpcd[DP_DPCD_REV] == DP_DPCD_REV_14)
+   intel_de_rmw(i915, CHICKEN_TRANS(crtc_state->cpu_transcoder),
+PREVENT_FEC_STALL_SIGNALING, 0);
+}
+
 static void intel_mst_post_disable_dp(struct intel_atomic_state *state,
  struct intel_encoder *encoder,
  const struct intel_crtc_state 
*old_crtc_state,
@@ -400,6 +424,8 @@ static void intel_mst_post_disable_dp(struct 
intel_atomic_state *state,
 
intel_disable_pipe(old_crtc_state);
 
+   intel_mst_unconfig_fec(encoder, old_crtc_state);
+
drm_dp_update_payload_part2(_dp->mst_mgr);
 
clear_act_sent(encoder, old_crtc_state);
@@ -563,6 +589,8 @@ static void intel_mst_enable_dp(struct intel_atomic_state 
*state,
 
drm_dp_update_payload_part2(_dp->mst_mgr);
 
+   intel_mst_config_fec(encoder, pipe_config);
+
intel_enable_pipe(pipe_config);
 
intel_crtc_vblank_on(pipe_config);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9ffd173f8b7f..6fe7aebed4f9 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8086,6 +8086,7 @@ enum {
 #define  HSW_FRAME_START_DELAY_MASK(3 << 27)
 #define  HSW_FRAME_START_DELAY(x)  ((x) << 27) /* 0-3 */
 #define  VSC_DATA_SEL_SOFTWARE_CONTROL (1 << 25) /* GLK and CNL+ */
+#define  PREVENT_FEC_STALL_SIGNALING   BIT(23)
 #define  DDI_TRAINING_OVERRIDE_ENABLE  (1 << 19)
 #define  DDI_TRAINING_OVERRIDE_VALUE   (1 << 18)
 #define  DDIE_TRAINING_OVERRIDE_ENABLE (1 << 17) /* CHICKEN_TRANS_A only */
-- 
2.31.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v1 1/1] drm/dp_mst: Use the correct max source link rate for i915

[why]
Previously used value was not safe to provide the correct value, i.e. it
could be 0 if not not configured, leading to no MST on this platform.

[how]
Do not use the value from BIOS, but from the structure populated at
encoder initialization time.

Fixes: 98025a62cb00 ("drm/dp_mst: Use Extended Base Receiver Capability DPCD 
space")
Signed-off-by: Nikola Cornij 
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c 
b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index bf7f8487945c..01a0ed99988f 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -942,7 +942,6 @@ intel_dp_mst_encoder_init(struct intel_digital_port 
*dig_port, int conn_base_id)
struct intel_dp *intel_dp = _port->dp;
enum port port = dig_port->base.port;
int ret;
-   int bios_max_link_rate;
 
if (!HAS_DP_MST(i915) || intel_dp_is_edp(intel_dp))
return 0;
@@ -957,11 +956,11 @@ intel_dp_mst_encoder_init(struct intel_digital_port 
*dig_port, int conn_base_id)
 
/* create encoders */
intel_dp_create_fake_mst_encoders(dig_port);
-   bios_max_link_rate = intel_bios_dp_max_link_rate(_port->base);
ret = drm_dp_mst_topology_mgr_init(_dp->mst_mgr, >drm,
   _dp->aux, 16, 3,
   (u8)dig_port->max_lanes,
-  (u8)(bios_max_link_rate / 27000), 
conn_base_id);
+  
(u8)(intel_dp->source_rates[intel_dp->num_source_rates - 1] / 27000),
+  conn_base_id);
if (ret)
return ret;
 
-- 
2.25.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v1 0/1] drm/dp_mst: Use the correct max source link rate for i915

This is a follow-up change to fix incorrectly used max link rate source
capability at MST init time.

Change history:

v1:
  - Initial

Nikola Cornij (1):
  drm/dp_mst: Use the correct max source link rate for i915

 drivers/gpu/drm/i915/display/intel_dp_mst.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

-- 
2.25.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v3 2/2] backlight: Add DisplayPort aux backlight driver

JFYI for anyone who is interested, I will be respinning my patches for adding
backlight helpers very soon since we've got pretty much all of the prep work
for it upstream now

On Mon, 2021-04-26 at 12:49 +0300, Jani Nikula wrote:
> On Mon, 26 Apr 2021, Rajeev Nandan  wrote:
> > Add backlight driver for the panels supporting backlight control
> > using DPCD registers on the DisplayPort aux channel.
> 
> No, please don't do this.
> 
> I wrote you last week in reply to v1 why I thought merging this would
> not be a good idea [1]. Why have you sent two versions since then
> without replying to me, or Cc'ing me or Lyude?
> 
> I think it's an even worse idea to merge this to
> drivers/video/backlight. With DP AUX backlight you can't pretend it's
> just an independent aux interface for backlight without everything else
> around it. It's not independent of eDP, and exposing it as a direct
> backlight sysfs interface bypasses the encoder.
> 
> And it still remains that there is existing DP AUX backlight code in
> use, in the tree, with more features than this, with plans and
> previously submitted patches to lift from one driver to drm core, and
> with patches to add support to another driver.
> 
> I don't say this lightly, or very often at all, but,
> 
> NAK.
> 
> 
> BR,
> Jani.
> 
> 
> [1] https://lore.kernel.org/dri-devel/871rb5bcf9@intel.com/
> 
> > 
> > Changes in v2:
> > - New (most of the code reused from drm_dp_aux_backlight.c of v1)
> > 
> > Changes in v3:
> > - Add missing ';' to fix module compilation (kernel test bot)
> > 
> > Signed-off-by: Rajeev Nandan 
> > ---
> >  drivers/video/backlight/Kconfig    |   7 +
> >  drivers/video/backlight/Makefile   |   1 +
> >  drivers/video/backlight/dp_aux_backlight.c | 245
> > +
> >  3 files changed, 253 insertions(+)
> >  create mode 100644 drivers/video/backlight/dp_aux_backlight.c
> > 
> > diff --git a/drivers/video/backlight/Kconfig
> > b/drivers/video/backlight/Kconfig
> > index d83c87b..82c88f0 100644
> > --- a/drivers/video/backlight/Kconfig
> > +++ b/drivers/video/backlight/Kconfig
> > @@ -456,6 +456,13 @@ config BACKLIGHT_LED
> >   If you have a LCD backlight adjustable by LED class driver, say
> > Y
> >   to enable this driver.
> >  
> > +config BACKLIGHT_DP_AUX
> > +   tristate "DisplayPort aux backlight driver"
> > +   depends on DRM && DRM_KMS_HELPER
> > +   help
> > + If you have a panel backlight controlled by DPCD registers
> > + on the DisplayPort aux channel, say Y to enable this driver.
> > +
> >  endif # BACKLIGHT_CLASS_DEVICE
> >  
> >  endmenu
> > diff --git a/drivers/video/backlight/Makefile
> > b/drivers/video/backlight/Makefile
> > index 685f3f1..ba23c7c 100644
> > --- a/drivers/video/backlight/Makefile
> > +++ b/drivers/video/backlight/Makefile
> > @@ -57,3 +57,4 @@ obj-$(CONFIG_BACKLIGHT_WM831X)+=
> > wm831x_bl.o
> >  obj-$(CONFIG_BACKLIGHT_ARCXCNN)+= arcxcnn_bl.o
> >  obj-$(CONFIG_BACKLIGHT_RAVE_SP)+= rave-sp-backlight.o
> >  obj-$(CONFIG_BACKLIGHT_LED)+= led_bl.o
> > +obj-$(CONFIG_BACKLIGHT_DP_AUX) += dp_aux_backlight.o
> > diff --git a/drivers/video/backlight/dp_aux_backlight.c
> > b/drivers/video/backlight/dp_aux_backlight.c
> > new file mode 100644
> > index ..3398383
> > --- /dev/null
> > +++ b/drivers/video/backlight/dp_aux_backlight.c
> > @@ -0,0 +1,245 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Backlight driver to control the brightness over DisplayPort aux
> > channel.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define DP_AUX_MAX_BRIGHTNESS  0x
> > +
> > +/**
> > + * struct dp_aux_backlight - DisplayPort aux backlight data
> > + * @dev: pointer to our device.
> > + * @aux: the DisplayPort aux channel.
> > + * @enable_gpio: the backlight enable gpio.
> > + * @enabled: true if backlight is enabled else false.
> > + */
> > +struct dp_aux_backlight {
> > +   struct device *dev;
> > +   struct drm_dp_aux *aux;
> > +   struct gpio_desc *enable_gpio;
> > +   bool enabled;
> > +};
> > +
> > +static struct drm_dp_aux *i2c_to_aux(struct i2c_adapter *i2c)
> > +{
> > +   return container_of(i2c, struct drm_dp_aux, ddc);
> > +}
> > +
> > +static int dp_aux_backlight_enable(struct dp_aux_backlight *aux_bl)
> > +{
> > +   u8 val = 0;
> > +   int ret;
> > +
> > +   if (aux_bl->enabled)
> > +   return 0;
> > +
> > +   /* Set backlight control mode */
> > +   ret = drm_dp_dpcd_readb(aux_bl->aux,
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> > +   );
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   val &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> > +   val |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > +   ret = drm_dp_dpcd_writeb(aux_bl->aux,
> >

Re: [Intel-gfx] [PATCH] drm/i915: Use might_alloc()

On Fri, Apr 30, 2021 at 12:31:27AM +0800, kernel test robot wrote:
> Hi Bernard,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on drm-intel/for-linux-next]
> [also build test ERROR on v5.12 next-20210429]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:
> https://github.com/0day-ci/linux/commits/Bernard-Zhao/drm-i915-Use-might_alloc/20210429-104516
> base:   git://anongit.freedesktop.org/drm-intel for-linux-next
> config: x86_64-rhel-8.3-kselftests (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce (this is a W=1 build):
> # 
> https://github.com/0day-ci/linux/commit/9fbd0c1741ce06241105d753ff3432ab55f3e94a
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review 
> Bernard-Zhao/drm-i915-Use-might_alloc/20210429-104516
> git checkout 9fbd0c1741ce06241105d753ff3432ab55f3e94a
> # save the attached .config to linux build tree
> make W=1 W=1 ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot 
> 
> All errors (new ones prefixed by >>):
> 
>drivers/gpu/drm/i915/i915_sw_fence.c: In function 
> '__i915_sw_fence_await_sw_fence':
> >> drivers/gpu/drm/i915/i915_sw_fence.c:344:2: error: implicit declaration of 
> >> function 'might_alloc'; did you mean 'might_lock'? 
> >> [-Werror=implicit-function-declaration]
>  344 |  might_alloc(gfp);
>  |  ^~~
>  |  might_lock
>cc1: some warnings being treated as errors

I think you're missing an include or something. The other patch you've
done seems good, I queued that up in drm-intel-gt-next for 5.14.

Thanks, Daniel

> 
> 
> vim +344 drivers/gpu/drm/i915/i915_sw_fence.c
> 
>335
>336static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence 
> *fence,
>337  struct i915_sw_fence 
> *signaler,
>338  wait_queue_entry_t 
> *wq, gfp_t gfp)
>339{
>340unsigned int pending;
>341unsigned long flags;
>342
>343debug_fence_assert(fence);
>  > 344might_alloc(gfp);
>345
>346if (i915_sw_fence_done(signaler)) {
>347i915_sw_fence_set_error_once(fence, 
> signaler->error);
>348return 0;
>349}
>350
>351debug_fence_assert(signaler);
>352
>353/* The dependency graph must be acyclic. */
>354if (unlikely(i915_sw_fence_check_if_after(fence, 
> signaler)))
>355return -EINVAL;
>356
>357pending = I915_SW_FENCE_FLAG_FENCE;
>358if (!wq) {
>359wq = kmalloc(sizeof(*wq), gfp);
>360if (!wq) {
>361if (!gfpflags_allow_blocking(gfp))
>362return -ENOMEM;
>363
>364i915_sw_fence_wait(signaler);
>365i915_sw_fence_set_error_once(fence, 
> signaler->error);
>366return 0;
>367}
>368
>369pending |= I915_SW_FENCE_FLAG_ALLOC;
>370}
>371
>372INIT_LIST_HEAD(>entry);
>373wq->flags = pending;
>374wq->func = i915_sw_fence_wake;
>375wq->private = fence;
>376
>377i915_sw_fence_await(fence);
>378
>379spin_lock_irqsave(>wait.lock, flags);
>380if (likely(!i915_sw_fence_done(signaler))) {
>381__add_wait_queue_entry_tail(>wait, 
> wq);
>382pending = 1;
>383} else {
>384i915_sw_fence_wake(wq, 0, signaler->error, 
> NULL);
>385pending = 0;
>386}
>387spin_unlock_irqrestore(>wait.lock, flags);
>388
>389return pending;
>390}
>391
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC v2] drm/i915: lpsp with hdmi/dp outputs

On Fri, Apr 30, 2021 at 05:23:55PM +0530, Anshuman Gupta wrote:
> DG1 and DISPLAY_VER=13 onwards Audio MMIO/VERBS lies in PG0 power
> well. So in order to detect audio capable DP/HDMI output it doesn't
> require to enable PG3 power well on DG1 and PG2 on DISPLAY_VER=13
> pltform. It will save the power when DP/HDMI outputs used as
> lpsp configuration.
> 
> B.Spec: 49233
> B.Spec: 49231
> 
> Cc: Ville Syrjälä 
> Cc: Kai Vehmanen 
> Cc: Uma Shankar 
> Signed-off-by: Anshuman Gupta 
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 6280ba7f4c17..16bfa7628970 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3536,8 +3536,13 @@ static u64 get_crtc_power_domains(struct 
> intel_crtc_state *crtc_state)
>   mask |= BIT_ULL(intel_encoder->power_domain);
>   }
>  
> - if (HAS_DDI(dev_priv) && crtc_state->has_audio)
> - mask |= BIT_ULL(POWER_DOMAIN_AUDIO);
> + if (HAS_DDI(dev_priv) && crtc_state->has_audio) {
> + /* if Audio mmio/verbs lies in PG0 */
> + if (IS_DG1(dev_priv) || DISPLAY_VER(dev_priv) >= 11)
> + mask |= BIT_ULL(POWER_DOMAIN_DISPLAY_CORE);
> + else
> + mask |= BIT_ULL(POWER_DOMAIN_AUDIO);

Power domains are supposed to be abstract. So if the audio stuff
is moving to a different power well then we just need to adjust
the domain for the relevant power wells.

> + }
>  
>   if (crtc_state->shared_dpll)
>   mask |= BIT_ULL(POWER_DOMAIN_DISPLAY_CORE);
> -- 
> 2.26.2

-- 
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

On Fri, Apr 30, 2021 at 6:57 PM Jason Ekstrand  wrote:
>
> On Fri, Apr 30, 2021 at 11:33 AM Daniel Vetter  wrote:
> >
> > On Fri, Apr 30, 2021 at 6:27 PM Jason Ekstrand  wrote:
> > >
> > > On Fri, Apr 30, 2021 at 1:53 AM Daniel Vetter  wrote:
> > > >
> > > > On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand  
> > > > wrote:
> > > > >
> > > > > On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter  wrote:
> > > > > >
> > > > > > On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:
> > > > > > > On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter  
> > > > > > > wrote:
> > > > > > > > On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:
> > > > > > > > > On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter 
> > > > > > > > >  wrote:
> > > > > > > > > > > + ret = set_proto_ctx_param(file_priv, pc, args);
> > > > > > > > > >
> > > > > > > > > > I think we should have a FIXME here of not allowing this on 
> > > > > > > > > > some future
> > > > > > > > > > platforms because just use CTX_CREATE_EXT.
> > > > > > > > >
> > > > > > > > > Done.
> > > > > > > > >
> > > > > > > > > > > + if (ret == -ENOTSUPP) {
> > > > > > > > > > > + /* Some params, specifically SSEU, can only 
> > > > > > > > > > > be set on fully
> > > > > > > > > >
> > > > > > > > > > I think this needs a FIXME: that this only holds during the 
> > > > > > > > > > conversion?
> > > > > > > > > > Otherwise we kinda have a bit a problem me thinks ...
> > > > > > > > >
> > > > > > > > > I'm not sure what you mean by that.
> > > > > > > >
> > > > > > > > Well I'm at least assuming that we wont have this case anymore, 
> > > > > > > > i.e.
> > > > > > > > there's only two kinds of parameters:
> > > > > > > > - those which are valid only on proto context
> > > > > > > > - those which are valid on both (like priority)
> > > > > > > >
> > > > > > > > This SSEU thing looks like a 3rd parameter, which is only valid 
> > > > > > > > on
> > > > > > > > finalized context. That feels all kinds of wrong. Will it stay? 
> > > > > > > > If yes
> > > > > > > > *ugh* and why?
> > > > > > >
> > > > > > > Because I was being lazy.  The SSEU stuff is a fairly complex 
> > > > > > > param to
> > > > > > > parse and it's always set live.  I can factor out the SSEU parsing
> > > > > > > code if you want and it shouldn't be too bad in the end.
> > > > > >
> > > > > > Yeah I think the special case here is a bit too jarring.
> > > > >
> > > > > I rolled a v5 that allows you to set SSEU as a create param.  I'm not
> > > > > a huge fan of that much code duplication for the SSEU set but I guess
> > > > > that's what we get for deciding to "unify" our context creation
> > > > > parameter path with our on-the-fly parameter path
> > > > >
> > > > > You can look at it here:
> > > > >
> > > > > https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588
> > > >
> > > > Hm yeah the duplication of the render engine check is a bit annoying.
> > > > What's worse, if you tthrow another set_engines on top it's probably
> > > > all wrong then. The old thing solved that by just throwing that
> > > > intel_context away.
> > >
> > > I think that's already mostly taken care of.  When set_engines
> > > happens, we throw away the old array of engines and start with a new
> > > one where everything has been memset to 0.  The one remaining problem
> > > is that, if userspace resets the engine set, we need to memset
> > > legacy_rcs_sseu to 0.  I've added that.
> > >
> > > > You're also not keeping the engine id in the proto ctx for this, so
> > > > there's probably some gaps there. We'd need to clear the SSEU if
> > > > userspace puts another context there. But also no userspace does that.
> > >
> > > Again, I think that's handled.  See above.
> > >
> > > > Plus cursory review of userspace show
> > > > - mesa doesn't set this
> > > > - compute sets its right before running the batch
> > > > - media sets it as the last thing of context creation
> > > >
> > > > So it's kinda not needed. But also we're asking umd to switch over to
> > > > CTX_CREATE_EXT, and if sseu doesn't work for that media team will be
> > > > puzzled. And we've confused them enough already with our uapis.
> > > >
> > > > Another idea: proto_set_sseu just stores the uapi struct and a note
> > > > that it's set, and checks nothing. To validate sseu on proto context
> > > > we do (but only when an sseu parameter is set):
> > > > 1. finalize the context
> > > > 2. call the real set_sseu for validation
> > > > 3. throw the finalized context away again, it was just for validating
> > > > the overall thing
> > > >
> > > > That way we don't have to consider all the interactions of setting
> > > > sseu and engines in any order on proto context, validation code is
> > > > guaranteed shared. Only downside is that there's a slight chance in
> > > > behaviour: SSEU, then setting another engine in that slot will fail
> > > > instead

Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

On Fri, Apr 30, 2021 at 11:33 AM Daniel Vetter  wrote:
>
> On Fri, Apr 30, 2021 at 6:27 PM Jason Ekstrand  wrote:
> >
> > On Fri, Apr 30, 2021 at 1:53 AM Daniel Vetter  wrote:
> > >
> > > On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand  
> > > wrote:
> > > >
> > > > On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter  wrote:
> > > > >
> > > > > On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:
> > > > > > On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter  
> > > > > > wrote:
> > > > > > > On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:
> > > > > > > > On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter 
> > > > > > > >  wrote:
> > > > > > > > > > + ret = set_proto_ctx_param(file_priv, pc, args);
> > > > > > > > >
> > > > > > > > > I think we should have a FIXME here of not allowing this on 
> > > > > > > > > some future
> > > > > > > > > platforms because just use CTX_CREATE_EXT.
> > > > > > > >
> > > > > > > > Done.
> > > > > > > >
> > > > > > > > > > + if (ret == -ENOTSUPP) {
> > > > > > > > > > + /* Some params, specifically SSEU, can only 
> > > > > > > > > > be set on fully
> > > > > > > > >
> > > > > > > > > I think this needs a FIXME: that this only holds during the 
> > > > > > > > > conversion?
> > > > > > > > > Otherwise we kinda have a bit a problem me thinks ...
> > > > > > > >
> > > > > > > > I'm not sure what you mean by that.
> > > > > > >
> > > > > > > Well I'm at least assuming that we wont have this case anymore, 
> > > > > > > i.e.
> > > > > > > there's only two kinds of parameters:
> > > > > > > - those which are valid only on proto context
> > > > > > > - those which are valid on both (like priority)
> > > > > > >
> > > > > > > This SSEU thing looks like a 3rd parameter, which is only valid on
> > > > > > > finalized context. That feels all kinds of wrong. Will it stay? 
> > > > > > > If yes
> > > > > > > *ugh* and why?
> > > > > >
> > > > > > Because I was being lazy.  The SSEU stuff is a fairly complex param 
> > > > > > to
> > > > > > parse and it's always set live.  I can factor out the SSEU parsing
> > > > > > code if you want and it shouldn't be too bad in the end.
> > > > >
> > > > > Yeah I think the special case here is a bit too jarring.
> > > >
> > > > I rolled a v5 that allows you to set SSEU as a create param.  I'm not
> > > > a huge fan of that much code duplication for the SSEU set but I guess
> > > > that's what we get for deciding to "unify" our context creation
> > > > parameter path with our on-the-fly parameter path
> > > >
> > > > You can look at it here:
> > > >
> > > > https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588
> > >
> > > Hm yeah the duplication of the render engine check is a bit annoying.
> > > What's worse, if you tthrow another set_engines on top it's probably
> > > all wrong then. The old thing solved that by just throwing that
> > > intel_context away.
> >
> > I think that's already mostly taken care of.  When set_engines
> > happens, we throw away the old array of engines and start with a new
> > one where everything has been memset to 0.  The one remaining problem
> > is that, if userspace resets the engine set, we need to memset
> > legacy_rcs_sseu to 0.  I've added that.
> >
> > > You're also not keeping the engine id in the proto ctx for this, so
> > > there's probably some gaps there. We'd need to clear the SSEU if
> > > userspace puts another context there. But also no userspace does that.
> >
> > Again, I think that's handled.  See above.
> >
> > > Plus cursory review of userspace show
> > > - mesa doesn't set this
> > > - compute sets its right before running the batch
> > > - media sets it as the last thing of context creation
> > >
> > > So it's kinda not needed. But also we're asking umd to switch over to
> > > CTX_CREATE_EXT, and if sseu doesn't work for that media team will be
> > > puzzled. And we've confused them enough already with our uapis.
> > >
> > > Another idea: proto_set_sseu just stores the uapi struct and a note
> > > that it's set, and checks nothing. To validate sseu on proto context
> > > we do (but only when an sseu parameter is set):
> > > 1. finalize the context
> > > 2. call the real set_sseu for validation
> > > 3. throw the finalized context away again, it was just for validating
> > > the overall thing
> > >
> > > That way we don't have to consider all the interactions of setting
> > > sseu and engines in any order on proto context, validation code is
> > > guaranteed shared. Only downside is that there's a slight chance in
> > > behaviour: SSEU, then setting another engine in that slot will fail
> > > instead of throwing the sseu parameters away. That's the right thing
> > > for CTX_CREATE_EXT anyway, and current userspace doesn't care.
> > >
> > > Thoughts?
> >
> > I thought about that.  The problem is that they can set_sseu multiple
> > times on different engines.  This means

Re: [Intel-gfx] [PATCH] drm/i915/display Try YCbCr420 color when RGB fails

On Thu, Apr 29, 2021 at 02:05:53PM +0200, Werner Sembach wrote:
> When encoder validation of a display mode fails, retry with less bandwidth
> heavy YCbCr420 color mode, if available. This enables some HDMI 1.4 setups
> to support 4k60Hz output, which previously failed silently.
> 
> AMDGPU had nearly the exact same issue. This problem description is
> therefore copied from my commit message of the AMDGPU patch.
> 
> On some setups, while the monitor and the gpu support display modes with
> pixel clocks of up to 600MHz, the link encoder might not. This prevents
> YCbCr444 and RGB encoding for 4k60Hz, but YCbCr420 encoding might still be
> possible. However, which color mode is used is decided before the link
> encoder capabilities are checked. This patch fixes the problem by retrying
> to find a display mode with YCbCr420 enforced and using it, if it is
> valid.
> 
> I'm not entierly sure if the second
> "if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv))" check in
> intel_hdmi_compute_config(...) after forcing ycbcr420 is necessary. I
> included it to better be safe then sorry.
> 
> Signed-off-by: Werner Sembach 
> Cc: 
> ---
> Rebased from 5.12 to drm-tip and resend to resolve merge conflict.
> 
> >From 876c1c8d970ff2a411ee8d08651bd4edbe9ecb3d Mon Sep 17 00:00:00 2001
> From: Werner Sembach 
> Date: Thu, 29 Apr 2021 13:59:30 +0200
> Subject: [PATCH] Retry using YCbCr420 encoding if clock setup for RGB fails
> 
> ---
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 80 +--
>  1 file changed, 60 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 46de56af33db..c9b5a7d7f9c6 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -1861,6 +1861,30 @@ static int intel_hdmi_port_clock(int clock, int bpc)
>   return clock * bpc / 8;
>  }
>  
> +static enum drm_mode_status
> +intel_hdmi_check_bpc(struct intel_hdmi *hdmi, int clock, bool has_hdmi_sink, 
> struct drm_i915_private *dev_priv)

Don't pass dev_priv. It can be extracted from the intel_hdmi.

The name of the function isn't really sitting super well with me.
I guess I'd just call it something like intel_hdmi_mode_clock_valid().

We should also split this big patch up into smaller parts. Just this
mechanical extraction of this function without any functional changes
could be a nice first patch in the series.

> +{
> + enum drm_mode_status status;
> +
> + /* check if we can do 8bpc */
> + status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 8),
> +true, has_hdmi_sink);
> +
> + if (has_hdmi_sink) {
> + /* if we can't do 8bpc we may still be able to do 12bpc */
> + if (status != MODE_OK && !HAS_GMCH(dev_priv))
> + status = hdmi_port_clock_valid(hdmi, 
> intel_hdmi_port_clock(clock, 12),
> +true, has_hdmi_sink);
> +
> + /* if we can't do 8,12bpc we may still be able to do 10bpc */
> + if (status != MODE_OK && DISPLAY_VER(dev_priv) >= 11)
> + status = hdmi_port_clock_valid(hdmi, 
> intel_hdmi_port_clock(clock, 10),
> +true, has_hdmi_sink);
> + }
> +
> + return status;
> +}
> +
>  static enum drm_mode_status
>  intel_hdmi_mode_valid(struct drm_connector *connector,
> struct drm_display_mode *mode)
> @@ -1891,23 +1915,18 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>   if (drm_mode_is_420_only(>display_info, mode))
>   clock /= 2;
>  
> - /* check if we can do 8bpc */
> - status = hdmi_port_clock_valid(hdmi, intel_hdmi_port_clock(clock, 8),
> -true, has_hdmi_sink);
> + status = intel_hdmi_check_bpc(hdmi, clock, has_hdmi_sink, dev_priv);
>  
> - if (has_hdmi_sink) {
> - /* if we can't do 8bpc we may still be able to do 12bpc */
> - if (status != MODE_OK && !HAS_GMCH(dev_priv))
> - status = hdmi_port_clock_valid(hdmi, 
> intel_hdmi_port_clock(clock, 12),
> -true, has_hdmi_sink);
> + if (status != MODE_OK) {
> + if (drm_mode_is_420_also(>display_info, mode)) {

We also need a connector->ycbcr_420_allowed check here.

> + /* if we can't do full color resolution we may still be 
> able to do reduced color resolution */
> + clock /= 2;
>  
> - /* if we can't do 8,12bpc we may still be able to do 10bpc */
> - if (status != MODE_OK && DISPLAY_VER(dev_priv) >= 11)
> - status = hdmi_port_clock_valid(hdmi, 
> intel_hdmi_port_clock(clock, 10),
> -true, has_hdmi_sink);
> + status =

Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

On Fri, Apr 30, 2021 at 6:27 PM Jason Ekstrand  wrote:
>
> On Fri, Apr 30, 2021 at 1:53 AM Daniel Vetter  wrote:
> >
> > On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand  
> > wrote:
> > >
> > > On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter  wrote:
> > > >
> > > > On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:
> > > > > On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter  wrote:
> > > > > > On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:
> > > > > > > On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter  
> > > > > > > wrote:
> > > > > > > > > + ret = set_proto_ctx_param(file_priv, pc, args);
> > > > > > > >
> > > > > > > > I think we should have a FIXME here of not allowing this on 
> > > > > > > > some future
> > > > > > > > platforms because just use CTX_CREATE_EXT.
> > > > > > >
> > > > > > > Done.
> > > > > > >
> > > > > > > > > + if (ret == -ENOTSUPP) {
> > > > > > > > > + /* Some params, specifically SSEU, can only be 
> > > > > > > > > set on fully
> > > > > > > >
> > > > > > > > I think this needs a FIXME: that this only holds during the 
> > > > > > > > conversion?
> > > > > > > > Otherwise we kinda have a bit a problem me thinks ...
> > > > > > >
> > > > > > > I'm not sure what you mean by that.
> > > > > >
> > > > > > Well I'm at least assuming that we wont have this case anymore, i.e.
> > > > > > there's only two kinds of parameters:
> > > > > > - those which are valid only on proto context
> > > > > > - those which are valid on both (like priority)
> > > > > >
> > > > > > This SSEU thing looks like a 3rd parameter, which is only valid on
> > > > > > finalized context. That feels all kinds of wrong. Will it stay? If 
> > > > > > yes
> > > > > > *ugh* and why?
> > > > >
> > > > > Because I was being lazy.  The SSEU stuff is a fairly complex param to
> > > > > parse and it's always set live.  I can factor out the SSEU parsing
> > > > > code if you want and it shouldn't be too bad in the end.
> > > >
> > > > Yeah I think the special case here is a bit too jarring.
> > >
> > > I rolled a v5 that allows you to set SSEU as a create param.  I'm not
> > > a huge fan of that much code duplication for the SSEU set but I guess
> > > that's what we get for deciding to "unify" our context creation
> > > parameter path with our on-the-fly parameter path
> > >
> > > You can look at it here:
> > >
> > > https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588
> >
> > Hm yeah the duplication of the render engine check is a bit annoying.
> > What's worse, if you tthrow another set_engines on top it's probably
> > all wrong then. The old thing solved that by just throwing that
> > intel_context away.
>
> I think that's already mostly taken care of.  When set_engines
> happens, we throw away the old array of engines and start with a new
> one where everything has been memset to 0.  The one remaining problem
> is that, if userspace resets the engine set, we need to memset
> legacy_rcs_sseu to 0.  I've added that.
>
> > You're also not keeping the engine id in the proto ctx for this, so
> > there's probably some gaps there. We'd need to clear the SSEU if
> > userspace puts another context there. But also no userspace does that.
>
> Again, I think that's handled.  See above.
>
> > Plus cursory review of userspace show
> > - mesa doesn't set this
> > - compute sets its right before running the batch
> > - media sets it as the last thing of context creation
> >
> > So it's kinda not needed. But also we're asking umd to switch over to
> > CTX_CREATE_EXT, and if sseu doesn't work for that media team will be
> > puzzled. And we've confused them enough already with our uapis.
> >
> > Another idea: proto_set_sseu just stores the uapi struct and a note
> > that it's set, and checks nothing. To validate sseu on proto context
> > we do (but only when an sseu parameter is set):
> > 1. finalize the context
> > 2. call the real set_sseu for validation
> > 3. throw the finalized context away again, it was just for validating
> > the overall thing
> >
> > That way we don't have to consider all the interactions of setting
> > sseu and engines in any order on proto context, validation code is
> > guaranteed shared. Only downside is that there's a slight chance in
> > behaviour: SSEU, then setting another engine in that slot will fail
> > instead of throwing the sseu parameters away. That's the right thing
> > for CTX_CREATE_EXT anyway, and current userspace doesn't care.
> >
> > Thoughts?
>
> I thought about that.  The problem is that they can set_sseu multiple
> times on different engines.  This means we'd have to effectively build
> up an arbitrary list of SSEU set operations and replay it.  I'm not
> sure how I feel about building up a big data structure.

Hm, but how does this work with proto ctx then? I've only seen a
single sseu param set in the patch you linked.

> > > I'm also going to send it

Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

On Fri, Apr 30, 2021 at 1:53 AM Daniel Vetter  wrote:
>
> On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand  wrote:
> >
> > On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter  wrote:
> > >
> > > On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:
> > > > On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter  wrote:
> > > > > On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:
> > > > > > On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter  
> > > > > > wrote:
> > > > > > > > + ret = set_proto_ctx_param(file_priv, pc, args);
> > > > > > >
> > > > > > > I think we should have a FIXME here of not allowing this on some 
> > > > > > > future
> > > > > > > platforms because just use CTX_CREATE_EXT.
> > > > > >
> > > > > > Done.
> > > > > >
> > > > > > > > + if (ret == -ENOTSUPP) {
> > > > > > > > + /* Some params, specifically SSEU, can only be 
> > > > > > > > set on fully
> > > > > > >
> > > > > > > I think this needs a FIXME: that this only holds during the 
> > > > > > > conversion?
> > > > > > > Otherwise we kinda have a bit a problem me thinks ...
> > > > > >
> > > > > > I'm not sure what you mean by that.
> > > > >
> > > > > Well I'm at least assuming that we wont have this case anymore, i.e.
> > > > > there's only two kinds of parameters:
> > > > > - those which are valid only on proto context
> > > > > - those which are valid on both (like priority)
> > > > >
> > > > > This SSEU thing looks like a 3rd parameter, which is only valid on
> > > > > finalized context. That feels all kinds of wrong. Will it stay? If yes
> > > > > *ugh* and why?
> > > >
> > > > Because I was being lazy.  The SSEU stuff is a fairly complex param to
> > > > parse and it's always set live.  I can factor out the SSEU parsing
> > > > code if you want and it shouldn't be too bad in the end.
> > >
> > > Yeah I think the special case here is a bit too jarring.
> >
> > I rolled a v5 that allows you to set SSEU as a create param.  I'm not
> > a huge fan of that much code duplication for the SSEU set but I guess
> > that's what we get for deciding to "unify" our context creation
> > parameter path with our on-the-fly parameter path
> >
> > You can look at it here:
> >
> > https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588
>
> Hm yeah the duplication of the render engine check is a bit annoying.
> What's worse, if you tthrow another set_engines on top it's probably
> all wrong then. The old thing solved that by just throwing that
> intel_context away.

I think that's already mostly taken care of.  When set_engines
happens, we throw away the old array of engines and start with a new
one where everything has been memset to 0.  The one remaining problem
is that, if userspace resets the engine set, we need to memset
legacy_rcs_sseu to 0.  I've added that.

> You're also not keeping the engine id in the proto ctx for this, so
> there's probably some gaps there. We'd need to clear the SSEU if
> userspace puts another context there. But also no userspace does that.

Again, I think that's handled.  See above.

> Plus cursory review of userspace show
> - mesa doesn't set this
> - compute sets its right before running the batch
> - media sets it as the last thing of context creation
>
> So it's kinda not needed. But also we're asking umd to switch over to
> CTX_CREATE_EXT, and if sseu doesn't work for that media team will be
> puzzled. And we've confused them enough already with our uapis.
>
> Another idea: proto_set_sseu just stores the uapi struct and a note
> that it's set, and checks nothing. To validate sseu on proto context
> we do (but only when an sseu parameter is set):
> 1. finalize the context
> 2. call the real set_sseu for validation
> 3. throw the finalized context away again, it was just for validating
> the overall thing
>
> That way we don't have to consider all the interactions of setting
> sseu and engines in any order on proto context, validation code is
> guaranteed shared. Only downside is that there's a slight chance in
> behaviour: SSEU, then setting another engine in that slot will fail
> instead of throwing the sseu parameters away. That's the right thing
> for CTX_CREATE_EXT anyway, and current userspace doesn't care.
>
> Thoughts?

I thought about that.  The problem is that they can set_sseu multiple
times on different engines.  This means we'd have to effectively build
up an arbitrary list of SSEU set operations and replay it.  I'm not
sure how I feel about building up a big data structure.

> > I'm also going to send it to trybot.
>
> If you resend pls include all my r-b, I think some got lost in v4.

I'll try and dig those up.

> Also, in the kernel at least we expect minimal commit message with a
> bit of context, there's no Part-of: link pointing at the entire MR
> with overview and discussion, the patchwork Link: we add is a pretty
> bad substitute. Some of the new patches in v4 are a bit too

[Intel-gfx] [RFC PATCH i-g-t] lib/i915/perf: Fix non-card0 processing

2021-04-30 Thread Janusz Krzysztofik

IGT i915/perf library functions now always operate on sysfs perf
attributes of card0 device node, no matter which DRM device fd a user
passes.  The intention was to always switch to primary device node if
a user passes a render device node fd, but that breaks handling of
non-card0 devices.

Instead of forcibly using DRM device minor number 0 when opening a
device sysfs area, convert device minor number of a user passed device
fd to the minor number of respective primary (cardX) device node.

Signed-off-by: Janusz Krzysztofik 
---
 lib/i915/perf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/i915/perf.c b/lib/i915/perf.c
index 56d5c0b3a..336824df7 100644
--- a/lib/i915/perf.c
+++ b/lib/i915/perf.c
@@ -376,8 +376,8 @@ open_master_sysfs_dir(int drm_fd)
if (fstat(drm_fd, ) || !S_ISCHR(st.st_mode))
 return -1;
 
-snprintf(path, sizeof(path), "/sys/dev/char/%d:0",
- major(st.st_rdev));
+snprintf(path, sizeof(path), "/sys/dev/char/%d:%d",
+ major(st.st_rdev), minor(st.st_rdev) & ~128);
 
return open(path, O_DIRECTORY);
 }
-- 
2.25.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 09/21] drm/i915/gem: Disallow creating contexts with too many engines

On Fri, Apr 30, 2021 at 6:40 AM Tvrtko Ursulin
 wrote:
>
>
> On 29/04/2021 20:16, Jason Ekstrand wrote:
> > On Thu, Apr 29, 2021 at 3:01 AM Tvrtko Ursulin
> >  wrote:
> >> On 28/04/2021 18:09, Jason Ekstrand wrote:
> >>> On Wed, Apr 28, 2021 at 9:26 AM Tvrtko Ursulin
> >>>  wrote:
>  On 28/04/2021 15:02, Daniel Vetter wrote:
> > On Wed, Apr 28, 2021 at 11:42:31AM +0100, Tvrtko Ursulin wrote:
> >>
> >> On 28/04/2021 11:16, Daniel Vetter wrote:
> >>> On Fri, Apr 23, 2021 at 05:31:19PM -0500, Jason Ekstrand wrote:
>  There's no sense in allowing userspace to create more engines than it
>  can possibly access via execbuf.
> 
>  Signed-off-by: Jason Ekstrand 
>  ---
>   drivers/gpu/drm/i915/gem/i915_gem_context.c | 7 +++
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
>  diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
>  b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>  index 5f8d0faf783aa..ecb3bf5369857 100644
>  --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>  +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>  @@ -1640,11 +1640,10 @@ set_engines(struct i915_gem_context *ctx,
>   return -EINVAL;
>   }
>  -  /*
>  -   * Note that I915_EXEC_RING_MASK limits execbuf to only using the
>  -   * first 64 engines defined here.
>  -   */
>   num_engines = (args->size - sizeof(*user)) / 
>  sizeof(*user->engines);
> >>>
> >>> Maybe add a comment like /* RING_MASK has not shift, so can be used
> >>> directly here */ since I had to check that :-)
> >>>
> >>> Same story about igt testcases needed, just to be sure.
> >>>
> >>> Reviewed-by: Daniel Vetter 
> >>
> >> I am not sure about the churn vs benefit ratio here. There are also 
> >> patches
> >> which extend the engine selection field in execbuf2 over the unused
> >> constants bits (with an explicit flag). So churn upstream and churn in
> >> internal (if interesting) for not much benefit.
> >
> > This isn't churn.
> >
> > This is "lock done uapi properly".
> >>>
> >>> Pretty much.
> >>
> >> Still haven't heard what concrete problems it solves.
> >>
>  IMO it is a "meh" patch. Doesn't fix any problems and will create work
>  for other people and man hours spent which no one will ever properly
>  account against.
> 
>  Number of contexts in the engine map should not really be tied to
>  execbuf2. As is demonstrated by the incoming work to address more than
>  63 engines, either as an extension to execbuf2 or future execbuf3.
> >>>
> >>> Which userspace driver has requested more than 64 engines in a single 
> >>> context?
> >>
> >> No need to artificially limit hardware capabilities in the uapi by
> >> implementing a policy in the kernel. Which will need to be
> >> removed/changed shortly anyway. This particular patch is work and
> >> creates more work (which other people who will get to fix the fallout
> >> will spend man hours to figure out what and why broke) for no benefit.
> >> Or you are yet to explain what the benefit is in concrete terms.
> >
> > You keep complaining about how much work it takes and yet I've spent
> > more time replying to your e-mails on this patch than I spent writing
> > the patch and the IGT test.  Also, if it takes so much time to add a
> > restriction, then why are we spending time figuring out how to modify
> > the uAPI to allow you to execbuf on a context with more than 64
> > engines?  If we're worried about engineering man-hours, then limiting
> > to 64 IS the pragmatic solution.
>
> a)
>
> Question of what problem does the patch fix is still unanswered.
>
> b)
>
> You miss the point. I'll continue in the next paragraph..
>
> >
> >> Why don't you limit it to number of physical engines then? Why don't you
> >> filter out duplicates? Why not limit the number of buffer objects per
> >> client or global based on available RAM + swap relative to minimum
> >> object size? Reductio ad absurdum yes, but illustrating the, in this
> >> case, a thin line between "locking down uapi" and adding too much policy
> >> where it is not appropriate.
> >
> > All this patch does is say that  you're not allowed to create a
> > context with more engines than the execbuf API will let you use.  We
> > already have an artificial limit.  All this does is push the error
> > handling further up the stack.  If someone comes up with a mechanism
> > to execbuf on engine 65 (they'd better have an open-source user if it
> > involves changing API), I'm very happy for them to bump this limit at
> > the same time.  It'll take them 5 minutes and it'll be something they
> > find while writing the IGT test.
>
> .. no it won't take five minutes.
>
> If I need to spell everything out - you will put this patch in,

Re: [Intel-gfx] [PATCH 03/21] drm/i915/gem: Set the watchdog timeout directly in intel_context_set_gem

On Fri, Apr 30, 2021 at 6:18 AM Tvrtko Ursulin
 wrote:
>
>
> On 29/04/2021 15:54, Jason Ekstrand wrote:
> > On Thu, Apr 29, 2021 at 3:04 AM Tvrtko Ursulin
> >  wrote:
> >>
> >>
> >> On 28/04/2021 18:24, Jason Ekstrand wrote:
> >>> On Wed, Apr 28, 2021 at 10:55 AM Tvrtko Ursulin
> >>>  wrote:
>  On 23/04/2021 23:31, Jason Ekstrand wrote:
> > Instead of handling it like a context param, unconditionally set it when
> > intel_contexts are created.  This doesn't fix anything but does simplify
> > the code a bit.
> >
> > Signed-off-by: Jason Ekstrand 
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_context.c   | 43 
> > +++
> > .../gpu/drm/i915/gem/i915_gem_context_types.h |  4 --
> > drivers/gpu/drm/i915/gt/intel_context_param.h |  3 +-
> > 3 files changed, 6 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
> > b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 35bcdeddfbf3f..1091cc04a242a 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -233,7 +233,11 @@ static void intel_context_set_gem(struct 
> > intel_context *ce,
> > intel_engine_has_timeslices(ce->engine))
> > __set_bit(CONTEXT_USE_SEMAPHORES, >flags);
> >
> > - intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us);
> > + if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&
> > + ctx->i915->params.request_timeout_ms) {
> > + unsigned int timeout_ms = 
> > ctx->i915->params.request_timeout_ms;
> > + intel_context_set_watchdog_us(ce, (u64)timeout_ms * 1000);
> 
>  Blank line between declarations and code please, or just lose the local.
> 
>  Otherwise looks okay. Slight change that same GEM context can now have a
>  mix of different request expirations isn't interesting I think. At least
>  the change goes away by the end of the series.
> >>>
> >>> In order for that to happen, I think you'd have to have a race between
> >>> CREATE_CONTEXT and someone smashing the request_timeout_ms param via
> >>> sysfs.  Or am I missing something?  Given that timeouts are really
> >>> per-engine anyway, I don't think we need to care too much about that.
> >>
> >> We don't care, no.
> >>
> >> For completeness only - by the end of the series it is what you say. But
> >> at _this_ point in the series though it is if modparam changes at any
> >> point between context create and replacing engines. Which is a change
> >> compared to before this patch, since modparam was cached in the GEM
> >> context so far. So one GEM context was a single request_timeout_ms.
> >
> > I've added the following to the commit message:
> >
> > It also means that sync files exported from different engines on a
> > SINGLE_TIMELINE context will have different fence contexts.  This is
> > visible to userspace if it looks at the obj_name field of
> > sync_fence_info.
> >
> > How's that sound?
>
> Wrong thread but sounds good.
>
> I haven't looked into the fence merge logic apart from noticing context
> is used there. So I'd suggest a quick look there on top, just to make
> sure merging logic does not hold any surprises if contexts start to
> differ. Probably just results with more inefficiency somewhere, in theory.

Looked at it yesterday.  It really does just create a fence array with
all the fences. :-)

--Jason
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/5] drm/i915: Use intel_de_wait_for_*() in cnl+ cdclk programming

From: Ville Syrjälä 

Replace the hand rolled PLL lock bit waits with intel_de_wait_for_*().

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
b/drivers/gpu/drm/i915/display/intel_cdclk.c
index ab68a2d70830..f8ed19205315 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1492,9 +1492,8 @@ static void cnl_cdclk_pll_disable(struct drm_i915_private 
*dev_priv)
 BXT_DE_PLL_PLL_ENABLE, 0);
 
/* Timeout 200us */
-   if (wait_for((intel_de_read(dev_priv, BXT_DE_PLL_ENABLE) & 
BXT_DE_PLL_LOCK) == 0, 1))
-   drm_err(_priv->drm,
-   "timeout waiting for CDCLK PLL unlock\n");
+   if (intel_de_wait_for_clear(dev_priv, BXT_DE_PLL_ENABLE, 
BXT_DE_PLL_LOCK, 1))
+   drm_err(_priv->drm, "timeout waiting for CDCLK PLL 
unlock\n");
 
dev_priv->cdclk.hw.vco = 0;
 }
@@ -1511,9 +1510,8 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private 
*dev_priv, int vco)
intel_de_write(dev_priv, BXT_DE_PLL_ENABLE, val);
 
/* Timeout 200us */
-   if (wait_for((intel_de_read(dev_priv, BXT_DE_PLL_ENABLE) & 
BXT_DE_PLL_LOCK) != 0, 1))
-   drm_err(_priv->drm,
-   "timeout waiting for CDCLK PLL lock\n");
+   if (intel_de_wait_for_set(dev_priv, BXT_DE_PLL_ENABLE, BXT_DE_PLL_LOCK, 
1))
+   drm_err(_priv->drm, "timeout waiting for CDCLK PLL lock\n");
 
dev_priv->cdclk.hw.vco = vco;
 }
-- 
2.26.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/5] drm/i915: Use intel_de_rmw() in bxt/glk/cnl+ cdclk programming

From: Ville Syrjälä 

Replace the hand rolled rmw sequences with intel_de_rmw().

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
b/drivers/gpu/drm/i915/display/intel_cdclk.c
index d7d051968af6..ab68a2d70830 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1472,12 +1472,9 @@ static void bxt_de_pll_disable(struct drm_i915_private 
*dev_priv)
 static void bxt_de_pll_enable(struct drm_i915_private *dev_priv, int vco)
 {
int ratio = DIV_ROUND_CLOSEST(vco, dev_priv->cdclk.hw.ref);
-   u32 val;
 
-   val = intel_de_read(dev_priv, BXT_DE_PLL_CTL);
-   val &= ~BXT_DE_PLL_RATIO_MASK;
-   val |= BXT_DE_PLL_RATIO(ratio);
-   intel_de_write(dev_priv, BXT_DE_PLL_CTL, val);
+   intel_de_rmw(dev_priv, BXT_DE_PLL_CTL,
+BXT_DE_PLL_RATIO_MASK, BXT_DE_PLL_RATIO(ratio));
 
intel_de_write(dev_priv, BXT_DE_PLL_ENABLE, BXT_DE_PLL_PLL_ENABLE);
 
@@ -1491,11 +1488,8 @@ static void bxt_de_pll_enable(struct drm_i915_private 
*dev_priv, int vco)
 
 static void cnl_cdclk_pll_disable(struct drm_i915_private *dev_priv)
 {
-   u32 val;
-
-   val = intel_de_read(dev_priv, BXT_DE_PLL_ENABLE);
-   val &= ~BXT_DE_PLL_PLL_ENABLE;
-   intel_de_write(dev_priv, BXT_DE_PLL_ENABLE, val);
+   intel_de_rmw(dev_priv, BXT_DE_PLL_ENABLE,
+BXT_DE_PLL_PLL_ENABLE, 0);
 
/* Timeout 200us */
if (wait_for((intel_de_read(dev_priv, BXT_DE_PLL_ENABLE) & 
BXT_DE_PLL_LOCK) == 0, 1))
-- 
2.26.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/5] drm/i915: Use intel_de_rmw() in skl cdclk programming

From: Ville Syrjälä 

Replace the hand rolled rmw sequences with intel_de_rmw().

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 25 ++
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 1ef8da2bf856..d7d051968af6 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -969,20 +969,16 @@ static u32 skl_dpll0_link_rate(struct drm_i915_private 
*dev_priv, int vco)
 
 static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
 {
-   u32 val;
-
-   val = intel_de_read(dev_priv, DPLL_CTRL1);
-
-   val &= ~(DPLL_CTRL1_HDMI_MODE(SKL_DPLL0) | DPLL_CTRL1_SSC(SKL_DPLL0) |
-DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0));
-   val |= DPLL_CTRL1_OVERRIDE(SKL_DPLL0);
-   val |= skl_dpll0_link_rate(dev_priv, vco);
-
-   intel_de_write(dev_priv, DPLL_CTRL1, val);
+   intel_de_rmw(dev_priv, DPLL_CTRL1,
+DPLL_CTRL1_HDMI_MODE(SKL_DPLL0) |
+DPLL_CTRL1_SSC(SKL_DPLL0) |
+DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0),
+DPLL_CTRL1_OVERRIDE(SKL_DPLL0) |
+skl_dpll0_link_rate(dev_priv, vco));
intel_de_posting_read(dev_priv, DPLL_CTRL1);
 
-   intel_de_write(dev_priv, LCPLL1_CTL,
-  intel_de_read(dev_priv, LCPLL1_CTL) | LCPLL_PLL_ENABLE);
+   intel_de_rmw(dev_priv, LCPLL1_CTL,
+0, LCPLL_PLL_ENABLE);
 
if (intel_de_wait_for_set(dev_priv, LCPLL1_CTL, LCPLL_PLL_LOCK, 5))
drm_err(_priv->drm, "DPLL0 not locked\n");
@@ -995,8 +991,9 @@ static void skl_dpll0_enable(struct drm_i915_private 
*dev_priv, int vco)
 
 static void skl_dpll0_disable(struct drm_i915_private *dev_priv)
 {
-   intel_de_write(dev_priv, LCPLL1_CTL,
-  intel_de_read(dev_priv, LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
+   intel_de_rmw(dev_priv, LCPLL1_CTL,
+LCPLL_PLL_ENABLE, 0);
+
if (intel_de_wait_for_clear(dev_priv, LCPLL1_CTL, LCPLL_PLL_LOCK, 1))
drm_err(_priv->drm, "Couldn't disable DPLL0\n");
 
-- 
2.26.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/5] drm/i915: Use intel_de_rmw() in bdw cdclk programming

From: Ville Syrjälä 

Replace the hand rolled rmw sequences with intel_de_rmw().

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
b/drivers/gpu/drm/i915/display/intel_cdclk.c
index d5314f9acf83..1ef8da2bf856 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -745,7 +745,6 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
  enum pipe pipe)
 {
int cdclk = cdclk_config->cdclk;
-   u32 val;
int ret;
 
if (drm_WARN(_priv->drm,
@@ -765,9 +764,8 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
return;
}
 
-   val = intel_de_read(dev_priv, LCPLL_CTL);
-   val |= LCPLL_CD_SOURCE_FCLK;
-   intel_de_write(dev_priv, LCPLL_CTL, val);
+   intel_de_rmw(dev_priv, LCPLL_CTL,
+0, LCPLL_CD_SOURCE_FCLK);
 
/*
 * According to the spec, it should be enough to poll for this 1 us.
@@ -777,14 +775,11 @@ static void bdw_set_cdclk(struct drm_i915_private 
*dev_priv,
LCPLL_CD_SOURCE_FCLK_DONE, 100))
drm_err(_priv->drm, "Switching to FCLK failed\n");
 
-   val = intel_de_read(dev_priv, LCPLL_CTL);
-   val &= ~LCPLL_CLK_FREQ_MASK;
-   val |= bdw_cdclk_freq_sel(cdclk);
-   intel_de_write(dev_priv, LCPLL_CTL, val);
+   intel_de_rmw(dev_priv, LCPLL_CTL,
+LCPLL_CLK_FREQ_MASK, bdw_cdclk_freq_sel(cdclk));
 
-   val = intel_de_read(dev_priv, LCPLL_CTL);
-   val &= ~LCPLL_CD_SOURCE_FCLK;
-   intel_de_write(dev_priv, LCPLL_CTL, val);
+   intel_de_rmw(dev_priv, LCPLL_CTL,
+LCPLL_CD_SOURCE_FCLK, 0);
 
if (wait_for_us((intel_de_read(dev_priv, LCPLL_CTL) &
 LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
-- 
2.26.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/5] drm/i915: Extract some helpers to compute cdclk register values

From: Ville Syrjälä 

Extract a few of the switch statements into helper functions to
reduce the pollution in the cdclk programming functions.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 186 ++---
 1 file changed, 88 insertions(+), 98 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c 
b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 1f0bd23bb883..d5314f9acf83 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -723,6 +723,23 @@ static void bdw_get_cdclk(struct drm_i915_private 
*dev_priv,
bdw_calc_voltage_level(cdclk_config->cdclk);
 }
 
+static u32 bdw_cdclk_freq_sel(int cdclk)
+{
+   switch (cdclk) {
+   default:
+   MISSING_CASE(cdclk);
+   fallthrough;
+   case 337500:
+   return LCPLL_CLK_FREQ_337_5_BDW;
+   case 45:
+   return LCPLL_CLK_FREQ_450;
+   case 54:
+   return LCPLL_CLK_FREQ_54O_BDW;
+   case 675000:
+   return LCPLL_CLK_FREQ_675_BDW;
+   }
+}
+
 static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
  const struct intel_cdclk_config *cdclk_config,
  enum pipe pipe)
@@ -762,25 +779,7 @@ static void bdw_set_cdclk(struct drm_i915_private 
*dev_priv,
 
val = intel_de_read(dev_priv, LCPLL_CTL);
val &= ~LCPLL_CLK_FREQ_MASK;
-
-   switch (cdclk) {
-   default:
-   MISSING_CASE(cdclk);
-   fallthrough;
-   case 337500:
-   val |= LCPLL_CLK_FREQ_337_5_BDW;
-   break;
-   case 45:
-   val |= LCPLL_CLK_FREQ_450;
-   break;
-   case 54:
-   val |= LCPLL_CLK_FREQ_54O_BDW;
-   break;
-   case 675000:
-   val |= LCPLL_CLK_FREQ_675_BDW;
-   break;
-   }
-
+   val |= bdw_cdclk_freq_sel(cdclk);
intel_de_write(dev_priv, LCPLL_CTL, val);
 
val = intel_de_read(dev_priv, LCPLL_CTL);
@@ -954,10 +953,8 @@ static void skl_set_preferred_cdclk_vco(struct 
drm_i915_private *dev_priv,
intel_update_max_cdclk(dev_priv);
 }
 
-static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
+static u32 skl_dpll0_link_rate(struct drm_i915_private *dev_priv, int vco)
 {
-   u32 val;
-
drm_WARN_ON(_priv->drm, vco != 810 && vco != 864);
 
/*
@@ -969,17 +966,22 @@ static void skl_dpll0_enable(struct drm_i915_private 
*dev_priv, int vco)
 * rate later on, with the constraint of choosing a frequency that
 * works with vco.
 */
+   if (vco == 864)
+   return DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080, 
SKL_DPLL0);
+   else
+   return DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_810, 
SKL_DPLL0);
+}
+
+static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
+{
+   u32 val;
+
val = intel_de_read(dev_priv, DPLL_CTRL1);
 
val &= ~(DPLL_CTRL1_HDMI_MODE(SKL_DPLL0) | DPLL_CTRL1_SSC(SKL_DPLL0) |
 DPLL_CTRL1_LINK_RATE_MASK(SKL_DPLL0));
val |= DPLL_CTRL1_OVERRIDE(SKL_DPLL0);
-   if (vco == 864)
-   val |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_1080,
-   SKL_DPLL0);
-   else
-   val |= DPLL_CTRL1_LINK_RATE(DPLL_CTRL1_LINK_RATE_810,
-   SKL_DPLL0);
+   val |= skl_dpll0_link_rate(dev_priv, vco);
 
intel_de_write(dev_priv, DPLL_CTRL1, val);
intel_de_posting_read(dev_priv, DPLL_CTRL1);
@@ -1006,6 +1008,29 @@ static void skl_dpll0_disable(struct drm_i915_private 
*dev_priv)
dev_priv->cdclk.hw.vco = 0;
 }
 
+static u32 skl_cdclk_freq_sel(struct drm_i915_private *dev_priv,
+ int cdclk, int vco)
+{
+   switch (cdclk) {
+   default:
+   drm_WARN_ON(_priv->drm,
+   cdclk != dev_priv->cdclk.hw.bypass);
+   drm_WARN_ON(_priv->drm, vco != 0);
+   fallthrough;
+   case 308571:
+   case 337500:
+   return CDCLK_FREQ_337_308;
+   case 45:
+   case 432000:
+   return CDCLK_FREQ_450_432;
+   case 54:
+   return CDCLK_FREQ_540;
+   case 617143:
+   case 675000:
+   return CDCLK_FREQ_675_617;
+   }
+}
+
 static void skl_set_cdclk(struct drm_i915_private *dev_priv,
  const struct intel_cdclk_config *cdclk_config,
  enum pipe pipe)
@@ -1036,29 +1061,7 @@ static void skl_set_cdclk(struct drm_i915_private 
*dev_priv,
return;
}
 
-   /* Choose frequency for this cdclk */
-   switch (cdclk) {
-   default:
-   drm_WARN_ON(_priv->drm,
-   cdclk !=

[Intel-gfx] [PATCH 0/5] drm/i915: cdclk cleanups

From: Ville Syrjälä 

A few easy cleanups to the cdclk code.

Ville Syrjälä (5):
  drm/i915: Extract some helpers to compute cdclk register values
  drm/i915: Use intel_de_rmw() in bdw cdclk programming
  drm/i915: Use intel_de_rmw() in skl cdclk programming
  drm/i915: Use intel_de_rmw() in bxt/glk/cnl+ cdclk programming
  drm/i915: Use intel_de_wait_for_*() in cnl+ cdclk programming

 drivers/gpu/drm/i915/display/intel_cdclk.c | 238 +
 1 file changed, 106 insertions(+), 132 deletions(-)

-- 
2.26.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 2/2] drm/i915: Include intel_de_{read, write}_fw() in i915_reg_rw traces

From: Ville Syrjälä 

We lost the i915_reg_rw tracepoint for a lot of display registers
when we switched from the heavyweight normal register accessors to
the lightweight _fw() variants. See eg. commit dd584fc0711a
("drm/i915: Use I915_READ_FW for plane updates").

Put the tracepoints back so that the register traces might
actually be useful. Hopefully these should be close to free
when the tracepoint is not enabled and thus not slow down
our vblank critical sections significantly.

v2: Copy paste the same-cacheline-hang warning from
intel_uncore.h (Anshuman)

Cc: Cooper Chiou 
Reviewed-by: Anshuman Gupta 
Reviewed-by: Jani Nikula 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_de.h | 41 -
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_de.h 
b/drivers/gpu/drm/i915/display/intel_de.h
index 00da10bf35f5..9d8c177aa228 100644
--- a/drivers/gpu/drm/i915/display/intel_de.h
+++ b/drivers/gpu/drm/i915/display/intel_de.h
@@ -8,6 +8,7 @@
 
 #include "i915_drv.h"
 #include "i915_reg.h"
+#include "i915_trace.h"
 #include "intel_uncore.h"
 
 static inline u32
@@ -22,26 +23,12 @@ intel_de_posting_read(struct drm_i915_private *i915, 
i915_reg_t reg)
intel_uncore_posting_read(>uncore, reg);
 }
 
-/* Note: read the warnings for intel_uncore_*_fw() functions! */
-static inline u32
-intel_de_read_fw(struct drm_i915_private *i915, i915_reg_t reg)
-{
-   return intel_uncore_read_fw(>uncore, reg);
-}
-
 static inline void
 intel_de_write(struct drm_i915_private *i915, i915_reg_t reg, u32 val)
 {
intel_uncore_write(>uncore, reg, val);
 }
 
-/* Note: read the warnings for intel_uncore_*_fw() functions! */
-static inline void
-intel_de_write_fw(struct drm_i915_private *i915, i915_reg_t reg, u32 val)
-{
-   intel_uncore_write_fw(>uncore, reg, val);
-}
-
 static inline void
 intel_de_rmw(struct drm_i915_private *i915, i915_reg_t reg, u32 clear, u32 set)
 {
@@ -69,4 +56,30 @@ intel_de_wait_for_clear(struct drm_i915_private *i915, 
i915_reg_t reg,
return intel_de_wait_for_register(i915, reg, mask, 0, timeout);
 }
 
+/*
+ * Unlocked mmio-accessors, think carefully before using these.
+ *
+ * Certain architectures will die if the same cacheline is concurrently 
accessed
+ * by different clients (e.g. on Ivybridge). Access to registers should
+ * therefore generally be serialised, by either the dev_priv->uncore.lock or
+ * a more localised lock guarding all access to that bank of registers.
+ */
+static inline u32
+intel_de_read_fw(struct drm_i915_private *i915, i915_reg_t reg)
+{
+   u32 val;
+
+   val = intel_uncore_read_fw(>uncore, reg);
+   trace_i915_reg_rw(false, reg, val, sizeof(val), true);
+
+   return val;
+}
+
+static inline void
+intel_de_write_fw(struct drm_i915_private *i915, i915_reg_t reg, u32 val)
+{
+   trace_i915_reg_rw(true, reg, val, sizeof(val), true);
+   intel_uncore_write_fw(>uncore, reg, val);
+}
+
 #endif /* __INTEL_DE_H__ */
-- 
2.26.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 1/2] drm/i915: Don't include intel_de.h from intel_display_types.h

From: Ville Syrjälä 

Hoist the intel_de.h include from intel_display_types.h one
level up. I need this in order to untangle the include order
so that I can add tracepoints into intel_de.h.

This little cocci script did most of the work for me:
@find@
@@
(
intel_de_read(...)
|
intel_de_read_fw(...)
|
intel_de_write(...)
|
intel_de_write_fw(...)
)

@has_include@
@@
(
 #include "intel_de.h"
|
 #include "display/intel_de.h"
)

@depends on find && !has_include@
@@
+ #include "intel_de.h"
  #include "intel_display_types.h"

@depends on find && !has_include@
@@
+ #include "display/intel_de.h"
  #include "display/intel_display_types.h"

Cc: Cooper Chiou 
Reviewed-by: Anshuman Gupta 
Reviewed-by: Jani Nikula 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/g4x_dp.c| 1 +
 drivers/gpu/drm/i915/display/g4x_hdmi.c  | 1 +
 drivers/gpu/drm/i915/display/i9xx_plane.c| 1 +
 drivers/gpu/drm/i915/display/icl_dsi.c   | 1 +
 drivers/gpu/drm/i915/display/intel_audio.c   | 1 +
 drivers/gpu/drm/i915/display/intel_cdclk.c   | 1 +
 drivers/gpu/drm/i915/display/intel_color.c   | 1 +
 drivers/gpu/drm/i915/display/intel_combo_phy.c   | 1 +
 drivers/gpu/drm/i915/display/intel_crt.c | 1 +
 drivers/gpu/drm/i915/display/intel_cursor.c  | 1 +
 drivers/gpu/drm/i915/display/intel_ddi.c | 1 +
 drivers/gpu/drm/i915/display/intel_ddi_buf_trans.c   | 1 +
 drivers/gpu/drm/i915/display/intel_display.c | 1 +
 drivers/gpu/drm/i915/display/intel_display_debugfs.c | 1 +
 drivers/gpu/drm/i915/display/intel_display_power.c   | 1 +
 drivers/gpu/drm/i915/display/intel_display_types.h   | 1 -
 drivers/gpu/drm/i915/display/intel_dp.c  | 1 +
 drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 1 +
 drivers/gpu/drm/i915/display/intel_dp_mst.c  | 1 +
 drivers/gpu/drm/i915/display/intel_dpio_phy.c| 1 +
 drivers/gpu/drm/i915/display/intel_dpll.c| 1 +
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c| 1 +
 drivers/gpu/drm/i915/display/intel_dsb.c | 1 +
 drivers/gpu/drm/i915/display/intel_dvo.c | 1 +
 drivers/gpu/drm/i915/display/intel_fbc.c | 1 +
 drivers/gpu/drm/i915/display/intel_fdi.c | 1 +
 drivers/gpu/drm/i915/display/intel_fifo_underrun.c   | 1 +
 drivers/gpu/drm/i915/display/intel_gmbus.c   | 1 +
 drivers/gpu/drm/i915/display/intel_hdcp.c| 1 +
 drivers/gpu/drm/i915/display/intel_hdmi.c| 1 +
 drivers/gpu/drm/i915/display/intel_lspcon.c  | 1 +
 drivers/gpu/drm/i915/display/intel_lvds.c| 1 +
 drivers/gpu/drm/i915/display/intel_overlay.c | 1 +
 drivers/gpu/drm/i915/display/intel_panel.c   | 1 +
 drivers/gpu/drm/i915/display/intel_pipe_crc.c| 1 +
 drivers/gpu/drm/i915/display/intel_pps.c | 1 +
 drivers/gpu/drm/i915/display/intel_psr.c | 1 +
 drivers/gpu/drm/i915/display/intel_sdvo.c| 1 +
 drivers/gpu/drm/i915/display/intel_sprite.c  | 1 +
 drivers/gpu/drm/i915/display/intel_tv.c  | 1 +
 drivers/gpu/drm/i915/display/intel_vdsc.c| 1 +
 drivers/gpu/drm/i915/display/intel_vrr.c | 1 +
 drivers/gpu/drm/i915/display/skl_scaler.c| 1 +
 drivers/gpu/drm/i915/display/skl_universal_plane.c   | 1 +
 drivers/gpu/drm/i915/display/vlv_dsi.c   | 1 +
 drivers/gpu/drm/i915/display/vlv_dsi_pll.c   | 1 +
 drivers/gpu/drm/i915/i915_irq.c  | 1 +
 drivers/gpu/drm/i915/intel_pm.c  | 1 +
 48 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/g4x_dp.c 
b/drivers/gpu/drm/i915/display/g4x_dp.c
index dfe3cf328d13..de0f358184aa 100644
--- a/drivers/gpu/drm/i915/display/g4x_dp.c
+++ b/drivers/gpu/drm/i915/display/g4x_dp.c
@@ -8,6 +8,7 @@
 #include "g4x_dp.h"
 #include "intel_audio.h"
 #include "intel_connector.h"
+#include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_dp.h"
 #include "intel_dp_link_training.h"
diff --git a/drivers/gpu/drm/i915/display/g4x_hdmi.c 
b/drivers/gpu/drm/i915/display/g4x_hdmi.c
index 78f93506ffaf..be352e9f0afc 100644
--- a/drivers/gpu/drm/i915/display/g4x_hdmi.c
+++ b/drivers/gpu/drm/i915/display/g4x_hdmi.c
@@ -8,6 +8,7 @@
 #include "g4x_hdmi.h"
 #include "intel_audio.h"
 #include "intel_connector.h"
+#include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_dpio_phy.h"
 #include "intel_fifo_underrun.h"
diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c 
b/drivers/gpu/drm/i915/display/i9xx_plane.c
index 393ef09ba0ac..9643c45a2209 100644
--- a/drivers/gpu/drm/i915/display/i9xx_plane.c
+++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
@@ -10,6 +10,7 @@
 
 #include "intel_atomic.h"
 #include "intel_atomic_plane.h"
+#include "intel_de.h"
 #include "intel_display_types.h"
 #include "intel_fb.h"
 #include "intel_sprite.h"
diff --git

Re: [Intel-gfx] [PATCH] drm/i915: Be more gentle with exiting non-persistent context

On Fri, Apr 30, 2021 at 3:27 PM Tvrtko Ursulin
 wrote:
> On 30/04/2021 12:48, Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 10:46:40AM +0100, Tvrtko Ursulin wrote:
> >> From: Tvrtko Ursulin 
> >>
> >> When a non-persistent context exits we currently mark it as banned in
> >> order to trigger fast termination of any outstanding GPU jobs it may have
> >> left running.
> >>
> >> In doing so we apply a very strict 1ms limit in which the left over job
> >> has to preempt before we issues an engine resets.
> >>
> >> Some workloads are not able to cleanly preempt in that time window and it
> >> can be argued that it would instead be better to give them a bit more
> >> grace since avoiding engine resets is generally preferrable.
> >
> > Can you pls explain here why this is preferrable?
>
> I think there's always the risk of an innocent request getting axed with
> preempt to busy being very asynchronous and also engine reset can
> sometimes fail as well.
>
> >> To achieve this the patch splits handling of banned contexts from simply
> >> exited non-persistent ones and then applies different timeouts for both
> >> and also extends the criteria which determines if a request should be
> >> scheduled back in after preemption or not.
> >>
> >> 15ms preempt timeout grace is given to exited non-persistent contexts
> >> which have been empirically tested to satisfy customers requirements
> >> and still provides reasonably quick cleanup post exit.
> >
> > Same here, a bit more detail on what exactly was the problem to be fixed
> > is needed.
>
> It is a bit multi-faceted. Start with how in some cultures errors
> messages are much bigger error flags than in others and much more
> difficult to hand-wave "oh that's not a problem really". The the
> previous considerations about why not avoid engine reset if we can. Add
> on top how non-persistent context exiting is not really an error worthy
> of a reset, *if* it exits cleanly reasonably quickly.
>
> You could say make it clean up for itself before it exits, not a kernel
> problem. But on the balance of everything, to me it sounds saleable to
> just give it some longer time compared to banned contexts, which are the
> unquestionably naughty/dangerous ones. Also, how fast non-persistent
> contexts will be cleaned up hasn't been defined really. As long as 15ms
> is an order of magnitude, plus some, shorter than the normal preempt
> timeout I think it is fine.

Yeah I think makes all sense, I just asked since the commit message
hinted at reasons but didn't explain any of them. I also assume that
generally the full reset flow is also on the order of 1ms (haven't
checked, but it's not cheap), so not being aggressive with engaging
that hammer is imo itself a very good reason to be a notch more lax
here.
-Danie

>
> Regards,
>
> Tvrtko
>
> P.S. Otherwise I plan to respin v2 with consolidated CONTEXT_SCHEDULABLE
> flag so fast paths do not have to do too many individual checks.
>
> > -Daniel
> >
> >>
> >> Signed-off-by: Tvrtko Ursulin 
> >> Cc: Chris Wilson 
> >> Cc: Zhen Han 
> >> ---
> >>   drivers/gpu/drm/i915/gem/i915_gem_context.c | 15 +--
> >>   drivers/gpu/drm/i915/gt/intel_context.h | 17 +
> >>   drivers/gpu/drm/i915/gt/intel_context_types.h   |  1 +
> >>   .../drm/i915/gt/intel_execlists_submission.c| 12 ++--
> >>   drivers/gpu/drm/i915/i915_request.c |  2 +-
> >>   5 files changed, 38 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
> >> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >> index fd8ee52e17a4..5a6eba1232cd 100644
> >> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >> @@ -426,7 +426,8 @@ static struct intel_engine_cs *active_engine(struct 
> >> intel_context *ce)
> >>  return engine;
> >>   }
> >>
> >> -static void kill_engines(struct i915_gem_engines *engines, bool ban)
> >> +static void
> >> +kill_engines(struct i915_gem_engines *engines, bool ban, bool persistent)
> >>   {
> >>  struct i915_gem_engines_iter it;
> >>  struct intel_context *ce;
> >> @@ -443,6 +444,8 @@ static void kill_engines(struct i915_gem_engines 
> >> *engines, bool ban)
> >>
> >>  if (ban && intel_context_set_banned(ce))
> >>  continue;
> >> +else if (!persistent && intel_context_set_non_persistent(ce))
> >> +continue;
> >>
> >>  /*
> >>   * Check the current active state of this context; if we
> >> @@ -454,7 +457,7 @@ static void kill_engines(struct i915_gem_engines 
> >> *engines, bool ban)
> >>  engine = active_engine(ce);
> >>
> >>  /* First attempt to gracefully cancel the context */
> >> -if (engine && !__cancel_engine(engine) && ban)
> >> +if (engine && !__cancel_engine(engine) && (ban || 
> >> !persistent))
> >>  /*
> >>

Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

On Fri, Apr 30, 2021 at 3:32 PM Hans de Goede  wrote:
>
> Hi,
>
> On 4/30/21 1:38 PM, Daniel Vetter wrote:
> > On Fri, Apr 30, 2021 at 1:28 PM Hans de Goede  wrote:
> >>
> >> Hi,
> >>
> >> On 4/29/21 9:09 PM, Daniel Vetter wrote:
> >>> On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
>  Hi,
> 
>  On 4/29/21 2:04 PM, Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
> >> On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
> >>> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
>  Userspace could hold open a reference to the connector->kdev device,
>  through e.g. holding a sysfs-atrtribute open after
>  drm_sysfs_connector_remove() has been called. In this case the 
>  connector
>  could be free-ed while the connector->kdev device's drvdata is still
>  pointing to it.
> 
>  Give drm_connector devices there own device type, which allows
>  us to specify our own release function and make 
>  drm_sysfs_connector_add()
>  take a reference on the connector object, and have the new release
>  function put the reference when the device is released.
> 
>  Giving drm_connector devices there own device type, will also allow
>  checking if a device is a drm_connector device with a
>  "if (device->type == _sysfs_device_connector)" check.
> 
>  Note that the setting of the name member of the device_type struct 
>  will
>  cause udev events for drm_connector-s to now contain 
>  DEVTYPE=drm_connector
>  as extra info. So this extends the uevent part of the userspace API.
> 
>  Signed-off-by: Hans de Goede 
> >>>
> >>> Are you sure? I thought sysfs is supposed to flush out any pending
> >>> operations (they complete fast) and handle open fd internally?
> >>
> >> Yes, it "should" :)
> >
> > Thanks for confirming my vague memories :-)
> >
> > Hans, pls drop this one.
> 
>  Please see my earlier reply to your review of this patch, it is
>  still needed but for a different reason:
> 
>  """
>  We still need this change though to make sure that the
>  "drm/connector: Add drm_connector_find_by_fwnode() function"
>  does not end up following a dangling drvdat pointer from one
>  if the drm_connector kdev-s.
> 
>  The class_dev_iter_init() in drm_connector_find_by_fwnode() gets
>  a reference on all devices and between getting that reference
>  and it calling drm_connector_get() - drm_connector_unregister()
>  may run and drop the possibly last reference to the
>  drm_connector object, freeing it and leaving the kdev's
>  drvdata as a dangling pointer.
>  """
> 
>  This is actually why I added it initially, and while adding it
>  I came up with this wrong theory of why it was necessary independently
>  of the drm_connector_find_by_fwnode() addition, sorry about that.
> >>>
> >>> Generally that's handled by a kref_get_unless_zero under the protection of
> >>> the lock which protects the weak reference. Which I think is the right
> >>> model here (at a glance at least) since this is a lookup function.
> >>
> >> I'm afraid that things are a bit more complicated here. The idea here
> >> is that we have a subsystem outside of the DRM subsystem which received
> >> a hotplug event for a drm-connector.  The only info which this subsystem
> >> has is a reference on the fwnode level (either through device-tree or
> >> to platform-code instantiating software-fwnode-s + links for this).
> >>
> >> So in order to deliver the hotplug event to the connector we need
> >> to lookup the connector by fwnode.
> >>
> >> I've chosen to implement this by iterating over all drm_class
> >> devices with a dev_type of drm_connector using class_dev_iter_init()
> >> and friends. This makes sure that we either get a reference to
> >> the device, or that we skip the device if it is being deleted.
> >>
> >> But this just gives us a reference to the connector->kdev, not
> >> to the connector itself. A pointer to the connector itself is stored
> >> as drvdata inside the device, but without taking a reference as
> >> this patch does, there is no guarantee that that pointer does not
> >> point to possibly free-ed mem.
> >>
> >> We could set drvdata to 0 from drm_sysfs_connector_remove()
> >> Before calling device_unregister(connector->kdev) and then do
> >> something like this inside drm_connector_find_by_fwnode():
> >>
> >> /*
> >>  * Lock the device to ensure we either see the drvdata == NULL
> >>  * set by drm_sysfs_connector_remove(); or we block the removal
> >>  * from continuing until we are done with the device.
> >>  */
> >> device_lock(dev);
> >> connector = dev_get_drvdata(dev);
> >> if (connector && connector->fwnode == fwnode) {
> >>

[Intel-gfx] [PATCH v2] drm/i915: Be more gentle with exiting non-persistent context

From: Tvrtko Ursulin 

When a non-persistent context exits we currently mark it as banned in
order to trigger fast termination of any outstanding GPU jobs it may have
left running.

In doing so we apply a very strict 1ms limit in which the left over job
has to preempt before we issues an engine resets.

Some workloads are not able to cleanly preempt in that time window and it
can be argued that it would instead be better to give them a bit more
grace since avoiding engine resets is generally preferrable.

To achieve this the patch splits handling of banned contexts from simply
exited non-persistent ones and then applies different timeouts for both
and also extends the criteria which determines if a request should be
scheduled back in after preemption or not.

15ms preempt timeout grace is given to exited non-persistent contexts
which have been empirically tested to satisfy customers requirements
and still provides reasonably quick cleanup post exit.

v2:
 * Streamline fast path checks.

Signed-off-by: Tvrtko Ursulin 
Cc: Chris Wilson 
Cc: Zhen Han 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c   | 16 +--
 drivers/gpu/drm/i915/gt/intel_context.c   |  2 ++
 drivers/gpu/drm/i915/gt/intel_context.h   | 20 +++
 drivers/gpu/drm/i915/gt/intel_context_types.h |  2 ++
 .../drm/i915/gt/intel_execlists_submission.c  | 11 --
 drivers/gpu/drm/i915/i915_request.c   |  2 +-
 6 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fd8ee52e17a4..090c891f029b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -426,7 +426,8 @@ static struct intel_engine_cs *active_engine(struct 
intel_context *ce)
return engine;
 }
 
-static void kill_engines(struct i915_gem_engines *engines, bool ban)
+static void
+kill_engines(struct i915_gem_engines *engines, bool ban, bool persistent)
 {
struct i915_gem_engines_iter it;
struct intel_context *ce;
@@ -443,6 +444,9 @@ static void kill_engines(struct i915_gem_engines *engines, 
bool ban)
 
if (ban && intel_context_set_banned(ce))
continue;
+   else if (!persistent &&
+intel_context_set_closed_non_persistent(ce))
+   continue;
 
/*
 * Check the current active state of this context; if we
@@ -454,7 +458,7 @@ static void kill_engines(struct i915_gem_engines *engines, 
bool ban)
engine = active_engine(ce);
 
/* First attempt to gracefully cancel the context */
-   if (engine && !__cancel_engine(engine) && ban)
+   if (engine && !__cancel_engine(engine) && (ban || !persistent))
/*
 * If we are unable to send a preemptive pulse to bump
 * the context from the GPU, we have to resort to a full
@@ -466,8 +470,6 @@ static void kill_engines(struct i915_gem_engines *engines, 
bool ban)
 
 static void kill_context(struct i915_gem_context *ctx)
 {
-   bool ban = (!i915_gem_context_is_persistent(ctx) ||
-   !ctx->i915->params.enable_hangcheck);
struct i915_gem_engines *pos, *next;
 
spin_lock_irq(>stale.lock);
@@ -480,7 +482,8 @@ static void kill_context(struct i915_gem_context *ctx)
 
spin_unlock_irq(>stale.lock);
 
-   kill_engines(pos, ban);
+   kill_engines(pos, !ctx->i915->params.enable_hangcheck,
+i915_gem_context_is_persistent(ctx));
 
spin_lock_irq(>stale.lock);
GEM_BUG_ON(i915_sw_fence_signaled(>fence));
@@ -526,7 +529,8 @@ static void engines_idle_release(struct i915_gem_context 
*ctx,
 
 kill:
if (list_empty(>link)) /* raced, already closed */
-   kill_engines(engines, true);
+   kill_engines(engines, true,
+i915_gem_context_is_persistent(ctx));
 
i915_sw_fence_commit(>fence);
 }
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c 
b/drivers/gpu/drm/i915/gt/intel_context.c
index 17cf2640b082..efbda677a0a8 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -374,6 +374,8 @@ intel_context_init(struct intel_context *ce, struct 
intel_engine_cs *engine)
ce->sseu = engine->sseu;
ce->ring = __intel_context_ring_size(SZ_4K);
 
+   __set_bit(CONTEXT_SCHEDULABLE, >flags);
+
ewma_runtime_init(>runtime.avg);
 
ce->vm = i915_vm_get(engine->gt->vm);
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h 
b/drivers/gpu/drm/i915/gt/intel_context.h
index f83a73a2b39f..a6d4c2d7f2f5 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -217,9 +217,29 @@ static inline bool

Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

2021-04-30 Thread Hans de Goede

p.s.

On 4/30/21 1:38 PM, Daniel Vetter wrote:

Offtopic:

> I'm also not sure why we have to use the kdev stuff here. For other
> random objects we need to look up we're building that functionality on
> that object. It means you need to keep another list_head around for
> that lookup, but that's really not a big cost. E.g. drm_bridge/panel
> work like that.

So I took a peek at the bridge/panel code and that actually seems to
have an issue with removal vs lookup. It is not even just a race,
it seems a lookup does not take a reference and there is nothing
stopping a user from doing an unbind or rmmod causing the panel
to be removed while other code which got a pointer to the panel
through of_drm_find_panel() will not be prepared to deal with
that pointer all of a sudden no longer being valid.

Now this would be a case of the user shooting his-self in the
foot (where as connectors can actually dynamically disappear
under normal circumstances), but ideally we really should do
better here.

Is there a TODO list somewhere for issues like this ?  Or shall
I submit a patch adding a FIXME comment, or is this considered
not worth the trouble of fixing it?

Regards,

Hans

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

2021-04-30 Thread Hans de Goede

Hi,

On 4/30/21 1:38 PM, Daniel Vetter wrote:
> On Fri, Apr 30, 2021 at 1:28 PM Hans de Goede  wrote:
>>
>> Hi,
>>
>> On 4/29/21 9:09 PM, Daniel Vetter wrote:
>>> On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
 Hi,

 On 4/29/21 2:04 PM, Daniel Vetter wrote:
> On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
>> On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
>>> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
 Userspace could hold open a reference to the connector->kdev device,
 through e.g. holding a sysfs-atrtribute open after
 drm_sysfs_connector_remove() has been called. In this case the 
 connector
 could be free-ed while the connector->kdev device's drvdata is still
 pointing to it.

 Give drm_connector devices there own device type, which allows
 us to specify our own release function and make 
 drm_sysfs_connector_add()
 take a reference on the connector object, and have the new release
 function put the reference when the device is released.

 Giving drm_connector devices there own device type, will also allow
 checking if a device is a drm_connector device with a
 "if (device->type == _sysfs_device_connector)" check.

 Note that the setting of the name member of the device_type struct will
 cause udev events for drm_connector-s to now contain 
 DEVTYPE=drm_connector
 as extra info. So this extends the uevent part of the userspace API.

 Signed-off-by: Hans de Goede 
>>>
>>> Are you sure? I thought sysfs is supposed to flush out any pending
>>> operations (they complete fast) and handle open fd internally?
>>
>> Yes, it "should" :)
>
> Thanks for confirming my vague memories :-)
>
> Hans, pls drop this one.

 Please see my earlier reply to your review of this patch, it is
 still needed but for a different reason:

 """
 We still need this change though to make sure that the
 "drm/connector: Add drm_connector_find_by_fwnode() function"
 does not end up following a dangling drvdat pointer from one
 if the drm_connector kdev-s.

 The class_dev_iter_init() in drm_connector_find_by_fwnode() gets
 a reference on all devices and between getting that reference
 and it calling drm_connector_get() - drm_connector_unregister()
 may run and drop the possibly last reference to the
 drm_connector object, freeing it and leaving the kdev's
 drvdata as a dangling pointer.
 """

 This is actually why I added it initially, and while adding it
 I came up with this wrong theory of why it was necessary independently
 of the drm_connector_find_by_fwnode() addition, sorry about that.
>>>
>>> Generally that's handled by a kref_get_unless_zero under the protection of
>>> the lock which protects the weak reference. Which I think is the right
>>> model here (at a glance at least) since this is a lookup function.
>>
>> I'm afraid that things are a bit more complicated here. The idea here
>> is that we have a subsystem outside of the DRM subsystem which received
>> a hotplug event for a drm-connector.  The only info which this subsystem
>> has is a reference on the fwnode level (either through device-tree or
>> to platform-code instantiating software-fwnode-s + links for this).
>>
>> So in order to deliver the hotplug event to the connector we need
>> to lookup the connector by fwnode.
>>
>> I've chosen to implement this by iterating over all drm_class
>> devices with a dev_type of drm_connector using class_dev_iter_init()
>> and friends. This makes sure that we either get a reference to
>> the device, or that we skip the device if it is being deleted.
>>
>> But this just gives us a reference to the connector->kdev, not
>> to the connector itself. A pointer to the connector itself is stored
>> as drvdata inside the device, but without taking a reference as
>> this patch does, there is no guarantee that that pointer does not
>> point to possibly free-ed mem.
>>
>> We could set drvdata to 0 from drm_sysfs_connector_remove()
>> Before calling device_unregister(connector->kdev) and then do
>> something like this inside drm_connector_find_by_fwnode():
>>
>> /*
>>  * Lock the device to ensure we either see the drvdata == NULL
>>  * set by drm_sysfs_connector_remove(); or we block the removal
>>  * from continuing until we are done with the device.
>>  */
>> device_lock(dev);
>> connector = dev_get_drvdata(dev);
>> if (connector && connector->fwnode == fwnode) {
>> drm_connector_get(connector);
>> found = connector;
>> }
>> device_unlock(dev);
> 
> Yes this is what I mean. Except not a drm_connector_get, but a
> kref_get_unless_zero. The connector might already be on it's way out,
> but the drvdata not yet cleared.

The function we

Re: [Intel-gfx] [PATCH] drm/i915: Be more gentle with exiting non-persistent context




On 30/04/2021 12:48, Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 10:46:40AM +0100, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

When a non-persistent context exits we currently mark it as banned in
order to trigger fast termination of any outstanding GPU jobs it may have
left running.

In doing so we apply a very strict 1ms limit in which the left over job
has to preempt before we issues an engine resets.

Some workloads are not able to cleanly preempt in that time window and it
can be argued that it would instead be better to give them a bit more
grace since avoiding engine resets is generally preferrable.


Can you pls explain here why this is preferrable?


I think there's always the risk of an innocent request getting axed with 
preempt to busy being very asynchronous and also engine reset can 
sometimes fail as well.



To achieve this the patch splits handling of banned contexts from simply
exited non-persistent ones and then applies different timeouts for both
and also extends the criteria which determines if a request should be
scheduled back in after preemption or not.

15ms preempt timeout grace is given to exited non-persistent contexts
which have been empirically tested to satisfy customers requirements
and still provides reasonably quick cleanup post exit.


Same here, a bit more detail on what exactly was the problem to be fixed
is needed.


It is a bit multi-faceted. Start with how in some cultures errors 
messages are much bigger error flags than in others and much more 
difficult to hand-wave "oh that's not a problem really". The the 
previous considerations about why not avoid engine reset if we can. Add 
on top how non-persistent context exiting is not really an error worthy 
of a reset, *if* it exits cleanly reasonably quickly.


You could say make it clean up for itself before it exits, not a kernel 
problem. But on the balance of everything, to me it sounds saleable to 
just give it some longer time compared to banned contexts, which are the 
unquestionably naughty/dangerous ones. Also, how fast non-persistent 
contexts will be cleaned up hasn't been defined really. As long as 15ms 
is an order of magnitude, plus some, shorter than the normal preempt 
timeout I think it is fine.


Regards,

Tvrtko

P.S. Otherwise I plan to respin v2 with consolidated CONTEXT_SCHEDULABLE 
flag so fast paths do not have to do too many individual checks.



-Daniel



Signed-off-by: Tvrtko Ursulin 
Cc: Chris Wilson 
Cc: Zhen Han 
---
  drivers/gpu/drm/i915/gem/i915_gem_context.c | 15 +--
  drivers/gpu/drm/i915/gt/intel_context.h | 17 +
  drivers/gpu/drm/i915/gt/intel_context_types.h   |  1 +
  .../drm/i915/gt/intel_execlists_submission.c| 12 ++--
  drivers/gpu/drm/i915/i915_request.c |  2 +-
  5 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fd8ee52e17a4..5a6eba1232cd 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -426,7 +426,8 @@ static struct intel_engine_cs *active_engine(struct 
intel_context *ce)
return engine;
  }
  
-static void kill_engines(struct i915_gem_engines *engines, bool ban)

+static void
+kill_engines(struct i915_gem_engines *engines, bool ban, bool persistent)
  {
struct i915_gem_engines_iter it;
struct intel_context *ce;
@@ -443,6 +444,8 @@ static void kill_engines(struct i915_gem_engines *engines, 
bool ban)
  
  		if (ban && intel_context_set_banned(ce))

continue;
+   else if (!persistent && intel_context_set_non_persistent(ce))
+   continue;
  
  		/*

 * Check the current active state of this context; if we
@@ -454,7 +457,7 @@ static void kill_engines(struct i915_gem_engines *engines, 
bool ban)
engine = active_engine(ce);
  
  		/* First attempt to gracefully cancel the context */

-   if (engine && !__cancel_engine(engine) && ban)
+   if (engine && !__cancel_engine(engine) && (ban || !persistent))
/*
 * If we are unable to send a preemptive pulse to bump
 * the context from the GPU, we have to resort to a full
@@ -466,8 +469,6 @@ static void kill_engines(struct i915_gem_engines *engines, 
bool ban)
  
  static void kill_context(struct i915_gem_context *ctx)

  {
-   bool ban = (!i915_gem_context_is_persistent(ctx) ||
-   !ctx->i915->params.enable_hangcheck);
struct i915_gem_engines *pos, *next;
  
  	spin_lock_irq(>stale.lock);

@@ -480,7 +481,8 @@ static void kill_context(struct i915_gem_context *ctx)
  
  		spin_unlock_irq(>stale.lock);
  
-		kill_engines(pos, ban);

+   kill_engines(pos, !ctx->i915->params.enable_hangcheck,
+i915_gem_context_is_persistent(ctx));

Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

On 30/04/2021 14:07, Daniel Vetter wrote:

On Fri, Apr 30, 2021 at 2:44 PM Tvrtko Ursulin
wrote:

On 30/04/2021 13:30, Daniel Vetter wrote:

On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
wrote:

On 30/04/2021 07:53, Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter wrote:

+ ret = set_proto_ctx_param(file_priv, pc, args);

I think we should have a FIXME here of not allowing this on some future
platforms because just use CTX_CREATE_EXT.

Done.

+ if (ret == -ENOTSUPP) {
+ /* Some params, specifically SSEU, can only be set on fully

I think this needs a FIXME: that this only holds during the conversion?
Otherwise we kinda have a bit a problem me thinks ...

I'm not sure what you mean by that.

Well I'm at least assuming that we wont have this case anymore, i.e.
there's only two kinds of parameters:
- those which are valid only on proto context
- those which are valid on both (like priority)

This SSEU thing looks like a 3rd parameter, which is only valid on
finalized context. That feels all kinds of wrong. Will it stay? If yes
*ugh* and why?

Because I was being lazy. The SSEU stuff is a fairly complex param to
parse and it's always set live. I can factor out the SSEU parsing
code if you want and it shouldn't be too bad in the end.

Yeah I think the special case here is a bit too jarring.

I rolled a v5 that allows you to set SSEU as a create param. I'm not
a huge fan of that much code duplication for the SSEU set but I guess
that's what we get for deciding to "unify" our context creation
parameter path with our on-the-fly parameter path

You can look at it here:

https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588

Hm yeah the duplication of the render engine check is a bit annoying.
What's worse, if you tthrow another set_engines on top it's probably
all wrong then. The old thing solved that by just throwing that
intel_context away.

You're also not keeping the engine id in the proto ctx for this, so
there's probably some gaps there. We'd need to clear the SSEU if
userspace puts another context there. But also no userspace does that.

Plus cursory review of userspace show
- mesa doesn't set this
- compute sets its right before running the batch
- media sets it as the last thing of context creation

Noticed a long sub-thread so looked inside..

SSEU is a really an interesting one.

For current userspace limiting to context creation is fine, since it is
only allowed for Icelake/VME use case. But if you notice the comment inside:

/* ABI restriction - VME use case only. */

It is a hint there was, or could be, more to this uapi than that.

And from memory I think limiting to creation time will nip the hopes
media had to use this dynamically on other platforms in the bud. So not
that good really. They had convincing numbers what gets significantly
better if we allowed dynamic control to this, just that as always, open
source userspace was not there so we never allowed it. However if you
come up with a new world order where it can only be done at context
creation, as said already, the possibility for that improvement (aka
further improving the competitive advantage) is most likely dashed.

Hm are you sure that this is create-time only? media-driver uses it
like that, but from my checking compute-runtime updates SSEU mode
before every execbuf call. So it very much looked like we have to keep
this dynamic.

Ah okay, I assumed it's more of the overall drive to eliminate
set_param. If sseu set_param stays then it's fine for what I had in mind.

Or do you mean this is defacto dead code? this = compute setting it
before every batch I mean here.

No idea, wasn't aware of the compute usage.

Before every execbuf is not very ideal though since we have to inject a
foreign context operation to update context image, which means stream of
work belonging to the context cannot be coalesced (assuming it could to
start with). There is also a hw cost to reconfigure the sseu which adds
latency on top.

They filter out no-op changes. I just meant that from look at
compute-runtime, it seems like sseu can change whenever.

i915 does it as well for good measure - since the penalty is global we
have to. So I guess they don't know when VME block will be used ie it is
per batch.

Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

On Fri, Apr 30, 2021 at 2:44 PM Tvrtko Ursulin
 wrote:
>
>
>
> On 30/04/2021 13:30, Daniel Vetter wrote:
> > On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
> >  wrote:
> >> On 30/04/2021 07:53, Daniel Vetter wrote:
> >>> On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand  
> >>> wrote:
> 
>  On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter  wrote:
> >
> > On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:
> >> On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter  wrote:
> >>> On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:
>  On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter  
>  wrote:
> >> + ret = set_proto_ctx_param(file_priv, pc, args);
> >
> > I think we should have a FIXME here of not allowing this on some 
> > future
> > platforms because just use CTX_CREATE_EXT.
> 
>  Done.
> 
> >> + if (ret == -ENOTSUPP) {
> >> + /* Some params, specifically SSEU, can only be set 
> >> on fully
> >
> > I think this needs a FIXME: that this only holds during the 
> > conversion?
> > Otherwise we kinda have a bit a problem me thinks ...
> 
>  I'm not sure what you mean by that.
> >>>
> >>> Well I'm at least assuming that we wont have this case anymore, i.e.
> >>> there's only two kinds of parameters:
> >>> - those which are valid only on proto context
> >>> - those which are valid on both (like priority)
> >>>
> >>> This SSEU thing looks like a 3rd parameter, which is only valid on
> >>> finalized context. That feels all kinds of wrong. Will it stay? If yes
> >>> *ugh* and why?
> >>
> >> Because I was being lazy.  The SSEU stuff is a fairly complex param to
> >> parse and it's always set live.  I can factor out the SSEU parsing
> >> code if you want and it shouldn't be too bad in the end.
> >
> > Yeah I think the special case here is a bit too jarring.
> 
>  I rolled a v5 that allows you to set SSEU as a create param.  I'm not
>  a huge fan of that much code duplication for the SSEU set but I guess
>  that's what we get for deciding to "unify" our context creation
>  parameter path with our on-the-fly parameter path
> 
>  You can look at it here:
> 
>  https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588
> >>>
> >>> Hm yeah the duplication of the render engine check is a bit annoying.
> >>> What's worse, if you tthrow another set_engines on top it's probably
> >>> all wrong then. The old thing solved that by just throwing that
> >>> intel_context away.
> >>>
> >>> You're also not keeping the engine id in the proto ctx for this, so
> >>> there's probably some gaps there. We'd need to clear the SSEU if
> >>> userspace puts another context there. But also no userspace does that.
> >>>
> >>> Plus cursory review of userspace show
> >>> - mesa doesn't set this
> >>> - compute sets its right before running the batch
> >>> - media sets it as the last thing of context creation
> >>
> >> Noticed a long sub-thread so looked inside..
> >>
> >> SSEU is a really an interesting one.
> >>
> >> For current userspace limiting to context creation is fine, since it is
> >> only allowed for Icelake/VME use case. But if you notice the comment 
> >> inside:
> >>
> >>  /* ABI restriction - VME use case only. */
> >>
> >> It is a hint there was, or could be, more to this uapi than that.
> >>
> >> And from memory I think limiting to creation time will nip the hopes
> >> media had to use this dynamically on other platforms in the bud. So not
> >> that good really. They had convincing numbers what gets significantly
> >> better if we allowed dynamic control to this, just that as always, open
> >> source userspace was not there so we never allowed it. However if you
> >> come up with a new world order where it can only be done at context
> >> creation, as said already, the possibility for that improvement (aka
> >> further improving the competitive advantage) is most likely dashed.
> >
> > Hm are you sure that this is create-time only? media-driver uses it
> > like that, but from my checking compute-runtime updates SSEU mode
> > before every execbuf call. So it very much looked like we have to keep
> > this dynamic.
>
> Ah okay, I assumed it's more of the overall drive to eliminate
> set_param. If sseu set_param stays then it's fine for what I had in mind.
>
> > Or do you mean this is defacto dead code? this = compute setting it
> > before every batch I mean here.
>
> No idea, wasn't aware of the compute usage.
>
> Before every execbuf is not very ideal though since we have to inject a
> foreign context operation to update context image, which means stream of
> work belonging to the context cannot be coalesced (assuming it could to
>

Re: [Intel-gfx] [PATCH v3 15/16] drm/i915/pxp: black pixels on pxp disabled

On Fri, Apr 30, 2021 at 07:12:53AM +, Gupta, Anshuman wrote:
> 
> 
> > -Original Message-
> > From: Ville Syrjälä 
> > Sent: Wednesday, April 28, 2021 12:26 AM
> > To: Gupta, Anshuman 
> > Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo ;
> > Gaurav, Kumar ; Shankar, Uma
> > ; Ceraolo Spurio, Daniele
> > 
> > Subject: Re: [PATCH v3 15/16] drm/i915/pxp: black pixels on pxp disabled
> > 
> > On Tue, Apr 27, 2021 at 04:15:04PM +0530, Anshuman Gupta wrote:
> > > When protected sufaces has flipped and pxp session is disabled,
> > > display black pixels by using plane color CTM correction.
> > >
> > > v2:
> > > - Display black pixels in aysnc flip too. 
> > 
> > We can't change any of that with an async flip.
> I was thinking of an scenario , when application flip the protected surfaces 
> with synchronous flips
> and driver has enable the plane decryption, can application issue an 
> intermediate async flip with
> protected surfaces afterwards ?
> If above is possible, is it possible to display black pixels in case of pxp 
> session invalidation at the time of
> Plane commit?   

We'll just have to refuse the async flip if the session has
been invalidated.

-- 
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support

On Fri, Apr 30, 2021 at 06:56:10AM +, Gupta, Anshuman wrote:
> 
> 
> > -Original Message-
> > From: Ceraolo Spurio, Daniele 
> > Sent: Thursday, April 29, 2021 2:10 AM
> > To: Ville Syrjälä 
> > Cc: Gupta, Anshuman ; Vivi, Rodrigo
> > ; intel-gfx@lists.freedesktop.org; Bommu, Krishnaiah
> > ; Gaurav, Kumar 
> > Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support
> > 
> > 
> > 
> > On 4/28/2021 1:04 PM, Ville Syrjälä wrote:
> > > On Wed, Apr 28, 2021 at 10:32:46AM -0700, Daniele Ceraolo Spurio wrote:
> > >>
> > >> On 4/28/2021 5:03 AM, Ville Syrjälä wrote:
> > >>> On Wed, Apr 28, 2021 at 11:25:23AM +, Gupta, Anshuman wrote:
> > > -Original Message-
> > > From: Ville Syrjälä 
> > > Sent: Wednesday, April 28, 2021 12:25 AM
> > > To: Gupta, Anshuman 
> > > Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo
> > > ; Bommu, Krishnaiah
> > > ; Huang Sean Z
> > > ; Gaurav, Kumar ;
> > > Ceraolo Spurio, Daniele 
> > > Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption
> > > support
> >  Thanks Ville for review, Below are some doubts with respect to pxp 
> >  state.
> > > On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote:
> > >> Add support to enable/disable PLANE_SURF Decryption Request bit.
> > >> It requires only to enable plane decryption support when
> > >> following condition met.
> > >> 1. PXP session is enabled.
> > >> 2. Buffer object is protected.
> > >>
> > >> v2:
> > >> - Used gen fb obj user_flags instead gem_object_metadata.
> > >> [Krishna]
> > >>
> > >> v3:
> > >> - intel_pxp_gem_object_status() API changes.
> > >>
> > >> v4: use intel_pxp_is_active (Daniele)
> > >>
> > >> v5: rebase and use the new protected object status checker
> > >> (Daniele)
> > >>
> > >> v6: used plane state for plane_decryption to handle async flip
> > >>   as suggested by Ville.
> > >>
> > >> Cc: Bommu Krishnaiah 
> > >> Cc: Huang Sean Z 
> > >> Cc: Gaurav Kumar 
> > >> Cc: Ville Syrjälä 
> > >> Signed-off-by: Anshuman Gupta 
> > >> Signed-off-by: Daniele Ceraolo Spurio
> > >> 
> > >> ---
> > >>.../gpu/drm/i915/display/intel_atomic_plane.c |  3 ++
> > >> drivers/gpu/drm/i915/display/intel_display.c  |  5 +++
> > >>.../drm/i915/display/intel_display_types.h|  3 ++
> > >>.../drm/i915/display/skl_universal_plane.c| 32 
> > >> +-
> > -
> > >>.../drm/i915/display/skl_universal_plane.h|  1 +
> > >>drivers/gpu/drm/i915/i915_reg.h   |  1 +
> > >>6 files changed, 42 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > >> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > >> index 7bfb26ca0bd0..7057077a2b71 100644
> > >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > >> @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct
> > > intel_atomic_state *state,
> > >>  intel_atomic_get_old_crtc_state(state, crtc);
> > >>  struct intel_crtc_state *new_crtc_state =
> > >>  intel_atomic_get_new_crtc_state(state, crtc);
> > >> +const struct drm_framebuffer *fb = new_plane_state->hw.fb;
> > >>
> > >>  if (new_crtc_state && new_crtc_state->bigjoiner_slave) {
> > >>  struct intel_plane *master_plane = @@ -409,6 +410,8
> > @@ int
> > >> intel_plane_atomic_check(struct
> > > intel_atomic_state *state,
> > >>  intel_plane_copy_uapi_to_hw_state(new_plane_state,
> > >>new_master_plane_state,
> > >>crtc);
> > >> +new_plane_state->plane_decryption =
> > >> +
> > i915_gem_object_has_valid_protection(intel_fb_obj(fb));
> > >>
> > >>  new_plane_state->uapi.visible = false;
> > >>  if (!new_crtc_state)
> > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > >> b/drivers/gpu/drm/i915/display/intel_display.c
> > >> index a10e26380ef3..55ab2d0b92d8 100644
> > >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > >> @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct
> > > intel_atomic_state *state)
> > >>  drm_dbg_kms(>drm, "Color range
> > cannot be
> > > changed in async flip\n");
> > >>  return -EINVAL;
> > >>  }
> > >> +
> > >> +/* plane decryption is allow to change only in
> > synchronous
> > >> +flips
> > > */
> > >> +if (old_plane_state->plane_decryption !=
> > new_plane_state-
> > >> plane_decryption)
> >

Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

On 30/04/2021 13:30, Daniel Vetter wrote:

On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
wrote:

On 30/04/2021 07:53, Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter wrote:

+ ret = set_proto_ctx_param(file_priv, pc, args);

I think we should have a FIXME here of not allowing this on some future
platforms because just use CTX_CREATE_EXT.

Done.

+ if (ret == -ENOTSUPP) {
+ /* Some params, specifically SSEU, can only be set on fully

I think this needs a FIXME: that this only holds during the conversion?
Otherwise we kinda have a bit a problem me thinks ...

I'm not sure what you mean by that.

This SSEU thing looks like a 3rd parameter, which is only valid on
finalized context. That feels all kinds of wrong. Will it stay? If yes
*ugh* and why?

Because I was being lazy. The SSEU stuff is a fairly complex param to
parse and it's always set live. I can factor out the SSEU parsing
code if you want and it shouldn't be too bad in the end.

Yeah I think the special case here is a bit too jarring.

You can look at it here:

https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588

Plus cursory review of userspace show
- mesa doesn't set this
- compute sets its right before running the batch
- media sets it as the last thing of context creation

Noticed a long sub-thread so looked inside..

SSEU is a really an interesting one.

For current userspace limiting to context creation is fine, since it is
only allowed for Icelake/VME use case. But if you notice the comment inside:

/* ABI restriction - VME use case only. */

It is a hint there was, or could be, more to this uapi than that.

Ah okay, I assumed it's more of the overall drive to eliminate
set_param. If sseu set_param stays then it's fine for what I had in mind.

Or do you mean this is defacto dead code? this = compute setting it
before every batch I mean here.

No idea, wasn't aware of the compute usage.

Anyway, I was only aware of the current media usage, which is static as
you say, and future/wishlist media usage, which would be dynamic, but a
complicated story to get right (partly due downsides mentioned in the
previous paragraph mean balancing benefit vs cost of dynamic sseu is not
easy).

Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915/fbc: Don't nuke manually around flips (rev3)

On Fri, Apr 30, 2021 at 06:31:49AM -, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/2] drm/i915/fbc: Don't nuke manually around 
> flips (rev3)
> URL   : https://patchwork.freedesktop.org/series/89517/
> State : success
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_10031_full -> Trybot_7704_full
> 
> 
> Summary
> ---
> 
>   **SUCCESS**
> 
>   No regressions found.
...
>   * igt@kms_draw_crc@draw-method-xrgb-render-untiled:
> - shard-iclb: [PASS][33] -> [FAIL][34] ([i915#52] / [i915#54]) +1 
> similar issue
>[33]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10031/shard-iclb4/igt@kms_draw_...@draw-method-xrgb-render-untiled.html
>[34]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_7704/shard-iclb8/igt@kms_draw_...@draw-method-xrgb-render-untiled.html

We have some seriously overzealous failure filtering going on here.
A lot of new failures on iclb with this run that all just got ignored.

We need to get this filtering reduced significantly or we'll end
up merging regressions. Not sure if it's possibly to clean it up
in any gradual way. Might just have to throw out everything related
to crc mismatches and start again from scratch. I suspect FIFO
underruns are in a similar situation.

And in the future really need to start tracking these types of
failures more or less by platform. I think currently we have just
several of these ancient meta-bugs where everything gets clumped
together, and thus no one can ever know what has been fixed and
what is still a problem on each platform.

-- 
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

On Fri, Apr 30, 2021 at 1:58 PM Tvrtko Ursulin
 wrote:
>
>
> On 30/04/2021 07:53, Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand  
> > wrote:
> >>
> >> On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter  wrote:
> >>>
> >>> On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:
>  On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter  wrote:
> > On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:
> >> On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter  wrote:
>  + ret = set_proto_ctx_param(file_priv, pc, args);
> >>>
> >>> I think we should have a FIXME here of not allowing this on some 
> >>> future
> >>> platforms because just use CTX_CREATE_EXT.
> >>
> >> Done.
> >>
>  + if (ret == -ENOTSUPP) {
>  + /* Some params, specifically SSEU, can only be set on 
>  fully
> >>>
> >>> I think this needs a FIXME: that this only holds during the 
> >>> conversion?
> >>> Otherwise we kinda have a bit a problem me thinks ...
> >>
> >> I'm not sure what you mean by that.
> >
> > Well I'm at least assuming that we wont have this case anymore, i.e.
> > there's only two kinds of parameters:
> > - those which are valid only on proto context
> > - those which are valid on both (like priority)
> >
> > This SSEU thing looks like a 3rd parameter, which is only valid on
> > finalized context. That feels all kinds of wrong. Will it stay? If yes
> > *ugh* and why?
> 
>  Because I was being lazy.  The SSEU stuff is a fairly complex param to
>  parse and it's always set live.  I can factor out the SSEU parsing
>  code if you want and it shouldn't be too bad in the end.
> >>>
> >>> Yeah I think the special case here is a bit too jarring.
> >>
> >> I rolled a v5 that allows you to set SSEU as a create param.  I'm not
> >> a huge fan of that much code duplication for the SSEU set but I guess
> >> that's what we get for deciding to "unify" our context creation
> >> parameter path with our on-the-fly parameter path
> >>
> >> You can look at it here:
> >>
> >> https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588
> >
> > Hm yeah the duplication of the render engine check is a bit annoying.
> > What's worse, if you tthrow another set_engines on top it's probably
> > all wrong then. The old thing solved that by just throwing that
> > intel_context away.
> >
> > You're also not keeping the engine id in the proto ctx for this, so
> > there's probably some gaps there. We'd need to clear the SSEU if
> > userspace puts another context there. But also no userspace does that.
> >
> > Plus cursory review of userspace show
> > - mesa doesn't set this
> > - compute sets its right before running the batch
> > - media sets it as the last thing of context creation
>
> Noticed a long sub-thread so looked inside..
>
> SSEU is a really an interesting one.
>
> For current userspace limiting to context creation is fine, since it is
> only allowed for Icelake/VME use case. But if you notice the comment inside:
>
> /* ABI restriction - VME use case only. */
>
> It is a hint there was, or could be, more to this uapi than that.
>
> And from memory I think limiting to creation time will nip the hopes
> media had to use this dynamically on other platforms in the bud. So not
> that good really. They had convincing numbers what gets significantly
> better if we allowed dynamic control to this, just that as always, open
> source userspace was not there so we never allowed it. However if you
> come up with a new world order where it can only be done at context
> creation, as said already, the possibility for that improvement (aka
> further improving the competitive advantage) is most likely dashed.

Hm are you sure that this is create-time only? media-driver uses it
like that, but from my checking compute-runtime updates SSEU mode
before every execbuf call. So it very much looked like we have to keep
this dynamic.

Or do you mean this is defacto dead code? this = compute setting it
before every batch I mean here.
-Daniel




--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [RFC v2] drm/i915: lpsp with hdmi/dp outputs

2021-04-30 Thread Anshuman Gupta

DG1 and DISPLAY_VER=13 onwards Audio MMIO/VERBS lies in PG0 power
well. So in order to detect audio capable DP/HDMI output it doesn't
require to enable PG3 power well on DG1 and PG2 on DISPLAY_VER=13
pltform. It will save the power when DP/HDMI outputs used as
lpsp configuration.

B.Spec: 49233
B.Spec: 49231

Cc: Ville Syrjälä 
Cc: Kai Vehmanen 
Cc: Uma Shankar 
Signed-off-by: Anshuman Gupta 
---
 drivers/gpu/drm/i915/display/intel_display.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 6280ba7f4c17..16bfa7628970 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -3536,8 +3536,13 @@ static u64 get_crtc_power_domains(struct 
intel_crtc_state *crtc_state)
mask |= BIT_ULL(intel_encoder->power_domain);
}
 
-   if (HAS_DDI(dev_priv) && crtc_state->has_audio)
-   mask |= BIT_ULL(POWER_DOMAIN_AUDIO);
+   if (HAS_DDI(dev_priv) && crtc_state->has_audio) {
+   /* if Audio mmio/verbs lies in PG0 */
+   if (IS_DG1(dev_priv) || DISPLAY_VER(dev_priv) >= 11)
+   mask |= BIT_ULL(POWER_DOMAIN_DISPLAY_CORE);
+   else
+   mask |= BIT_ULL(POWER_DOMAIN_AUDIO);
+   }
 
if (crtc_state->shared_dpll)
mask |= BIT_ULL(POWER_DOMAIN_DISPLAY_CORE);
-- 
2.26.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] REGRESSION with 5.12: Suspend not working on Toshiba notebook

2021-04-30 Thread Andreas Friedrich

On Fri, Apr 30, 2021 at 11:31:47AM +0300, Joonas Lahtinen wrote:

Hello Joonas,

thank you for your quick response.
...
> That is a merge commit, it doesn't itself change anything as there were no
> conflicts. It just indicates that two trees got merged.
...
> If you are able to provide a bisect to a one patch, please do report it
> as a bug, let's then take it from there.

I have bisect the kernel 3 times and it always leads me to the wrong
end. Because not every suspend results in a frozen system, maybe I
have marked a 'bisect good' although it was bad.

What I surely can say is, that kernel 5.11.16 works fine and 5.12-rc1
(v5.12-rc1-dontuse) does not.

However, on the bisecting path I saw the following commits:
  [41a9c75d0acf23f33f012d3f9535de9e9b631051] drm/i915/gem: Move stolen node 
into GEM object union
  [d82afcf9caaac26ce264255bca9dacf30f41] Merge tag 
'drm-intel-gt-next-2021-01-21-1' of git://anongit.freedesktop.org/drm/drm-intel 
into drm-next
  [885e1938452fc7fc37a3051d76e1ddb7ead099fa] drm/i915/gvt: statically set 
F_CMD_WRITE_PATCH flag
  [a2dd2ff50cde3cbbeecec72225bb18582b291f14] drm/i915/gt: Skip over completed 
active execlists, again
  [02dd2b12a685944c4d52c569d05f636372a7b6c7] drm/i915/gvt: unify lri cmd 
handler and mmio handlers
  [69b4b99842201bc24c98ba66b922d8879e190483] drm/i915/gvt: Add missing forward 
decl of intel_vgpu for HDRTEST
  [c071a6c0fef0fade787d827c7fc0e07481512326] Merge tag 'gvt-gt-next-2021-01-18' 
of https://github.com/intel/gvt-linux into drm-intel-gt-next

So I think it's a drm-i915 issue. I have communicated with David
Airlie (airl...@redhat.com) and he also bets on an i915 bug.
...
> https://01.org/linuxgraphics/documentation/how-report-bugs

Following https://gitlab.freedesktop.org/drm/intel/-/wikis/How-to-file-i915-bugs
I can give you the following infos:
  - After calling 'echo mem > /sys/power/state' the system gets frozen
(the screen is dimmed but the CPU fan is still running). The
system is inaccessible from remote. The only way to get it working
again is to hard power off and on.
  - One out of 5 suspend tries will cause the issue, mostly the first one.
  - Notebook: Old Toshiba Tecra A10 from 2009.
  - x86_64, 5.12-rc1, Debian GNU/Linux 10 (buster), see dmidecode.txt.gz
and 5.12.config.gz
  - Because the system is frozen, I cannot read /sys/class/drm/card0/error
before rebooting.

Please let me know if I can do anyting else to solve the problem.

Best regards
Andreas Friedrich


dmidecode.txt.gz
Description: application/gzip


5.12.config.gz
Description: application/gzip
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [v3,1/5] drm/connector: Create a helper to attach the hdr_output_metadata property

== Series Details ==

Series: series starting with [v3,1/5] drm/connector: Create a helper to attach 
the hdr_output_metadata property
URL   : https://patchwork.freedesktop.org/series/89690/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10032_full -> Patchwork_20043_full


Summary
---

  **SUCCESS**

  No regressions found.

  

Known issues


  Here are the changes found in Patchwork_20043_full that come from known 
issues:

### IGT changes ###

 Issues hit 

  * igt@feature_discovery@psr2:
- shard-iclb: [PASS][1] -> [SKIP][2] ([i915#658])
   [1]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/shard-iclb2/igt@feature_discov...@psr2.html
   [2]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-iclb3/igt@feature_discov...@psr2.html

  * igt@gem_ctx_persistence@clone:
- shard-snb:  NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#1099]) +5 
similar issues
   [3]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-snb6/igt@gem_ctx_persiste...@clone.html

  * igt@gem_eio@unwedge-stress:
- shard-iclb: [PASS][4] -> [TIMEOUT][5] ([i915#2369] / [i915#2481] 
/ [i915#3070])
   [4]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/shard-iclb4/igt@gem_...@unwedge-stress.html
   [5]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-iclb4/igt@gem_...@unwedge-stress.html
- shard-snb:  NOTRUN -> [FAIL][6] ([i915#3354])
   [6]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-snb7/igt@gem_...@unwedge-stress.html

  * igt@gem_exec_fair@basic-flow@rcs0:
- shard-tglb: [PASS][7] -> [FAIL][8] ([i915#2842])
   [7]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/shard-tglb3/igt@gem_exec_fair@basic-f...@rcs0.html
   [8]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-tglb6/igt@gem_exec_fair@basic-f...@rcs0.html

  * igt@gem_exec_fair@basic-pace@rcs0:
- shard-kbl:  [PASS][9] -> [FAIL][10] ([i915#2842])
   [9]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/shard-kbl7/igt@gem_exec_fair@basic-p...@rcs0.html
   [10]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-kbl1/igt@gem_exec_fair@basic-p...@rcs0.html

  * igt@gem_exec_fair@basic-throttle@rcs0:
- shard-iclb: [PASS][11] -> [FAIL][12] ([i915#2849])
   [11]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/shard-iclb5/igt@gem_exec_fair@basic-throt...@rcs0.html
   [12]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-iclb1/igt@gem_exec_fair@basic-throt...@rcs0.html

  * igt@gem_exec_reloc@basic-wide-active@rcs0:
- shard-snb:  NOTRUN -> [FAIL][13] ([i915#2389]) +2 similar issues
   [13]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-snb2/igt@gem_exec_reloc@basic-wide-act...@rcs0.html

  * igt@gem_exec_schedule@deep@vecs0:
- shard-skl:  [PASS][14] -> [INCOMPLETE][15] ([i915#2624])
   [14]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/shard-skl10/igt@gem_exec_schedule@d...@vecs0.html
   [15]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-skl3/igt@gem_exec_schedule@d...@vecs0.html

  * igt@gem_exec_whisper@basic-fds-forked-all:
- shard-glk:  [PASS][16] -> [DMESG-WARN][17] ([i915#118] / 
[i915#95])
   [16]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/shard-glk8/igt@gem_exec_whis...@basic-fds-forked-all.html
   [17]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-glk6/igt@gem_exec_whis...@basic-fds-forked-all.html

  * igt@gem_mmap_gtt@big-copy-xy:
- shard-skl:  [PASS][18] -> [FAIL][19] ([i915#307])
   [18]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/shard-skl8/igt@gem_mmap_...@big-copy-xy.html
   [19]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-skl8/igt@gem_mmap_...@big-copy-xy.html

  * igt@gem_mmap_gtt@cpuset-medium-copy:
- shard-iclb: [PASS][20] -> [FAIL][21] ([i915#307]) +1 similar issue
   [20]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/shard-iclb2/igt@gem_mmap_...@cpuset-medium-copy.html
   [21]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-iclb3/igt@gem_mmap_...@cpuset-medium-copy.html

  * igt@gem_pwrite@basic-exhaustion:
- shard-snb:  NOTRUN -> [WARN][22] ([i915#2658])
   [22]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-snb6/igt@gem_pwr...@basic-exhaustion.html

  * igt@gem_userptr_blits@vma-merge:
- shard-skl:  NOTRUN -> [FAIL][23] ([i915#3318])
   [23]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/shard-skl9/igt@gem_userptr_bl...@vma-merge.html

  * igt@i915_suspend@sysfs-reader:
- shard-kbl:  [PASS][24] -> [INCOMPLETE][25] ([i915#155])
   [24]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/shard-kbl7/igt@i915_susp...@sysfs-reader.html
   [25]:

Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation

On 30/04/2021 07:53, Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 11:35 PM Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 2:07 PM Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 02:01:16PM -0500, Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 1:56 PM Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 01:16:04PM -0500, Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 10:51 AM Daniel Vetter wrote:

+ ret = set_proto_ctx_param(file_priv, pc, args);

I think we should have a FIXME here of not allowing this on some future
platforms because just use CTX_CREATE_EXT.

Done.

+ if (ret == -ENOTSUPP) {
+ /* Some params, specifically SSEU, can only be set on fully

I think this needs a FIXME: that this only holds during the conversion?
Otherwise we kinda have a bit a problem me thinks ...

I'm not sure what you mean by that.

This SSEU thing looks like a 3rd parameter, which is only valid on
finalized context. That feels all kinds of wrong. Will it stay? If yes
*ugh* and why?

Because I was being lazy. The SSEU stuff is a fairly complex param to
parse and it's always set live. I can factor out the SSEU parsing
code if you want and it shouldn't be too bad in the end.

Yeah I think the special case here is a bit too jarring.

You can look at it here:

https://gitlab.freedesktop.org/jekstrand/linux/-/commit/c805f424a3374b2de405b7fc651eab551df2cdaf#474deb1194892a272db022ff175872d42004dfda_283_588

Plus cursory review of userspace show
- mesa doesn't set this
- compute sets its right before running the batch
- media sets it as the last thing of context creation

Noticed a long sub-thread so looked inside..

SSEU is a really an interesting one.

For current userspace limiting to context creation is fine, since it is
only allowed for Icelake/VME use case. But if you notice the comment inside:

/* ABI restriction - VME use case only. */

It is a hint there was, or could be, more to this uapi than that.

Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/overlay: Fix active retire callback alignment

On Thu, Apr 29, 2021 at 06:34:51PM +0100, Tvrtko Ursulin wrote:
> 
> On 29/04/2021 17:31, Ville Syrjälä wrote:
> > On Thu, Apr 29, 2021 at 09:35:29AM +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin 
> > > 
> > > __i915_active_call annotation is required on the retire callback to ensure
> > > correct function alignment.
> > > 
> > > Signed-off-by: Tvrtko Ursulin 
> > > Fixes: a21ce8ad12d2 ("drm/i915/overlay: Switch to using i915_active 
> > > tracking")
> > > Cc: Chris Wilson 
> > > Cc: Matthew Auld 
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c 
> > > b/drivers/gpu/drm/i915/display/intel_overlay.c
> > > index fffbde4256db..428819ba18dd 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> > > @@ -383,7 +383,7 @@ static void intel_overlay_off_tail(struct 
> > > intel_overlay *overlay)
> > >   i830_overlay_clock_gating(dev_priv, true);
> > >   }
> > > -static void
> > > +__i915_active_call static void
> > 
> > Am I blind or are we just packing flag bits into a pointer, passing
> > that to a function, and then immediately unpack the bits again in
> > said function? Why not just pass the flags explicitly?
> > 
> > Looks like you missed auto_retire()?
> 
> Yeah, both points already either fixed or under consideration:
> https://patchwork.freedesktop.org/patch/431473/?series=89623=1
> 
> I left the splitting up vfunc vs flags for later.

Yeah pls remove this pointer packing asap.

This is yet another case of pointless complications and fragility in the
code base for not reason at all, and it needs to go.

I'll file a jira and assign to Matt Auld, since he reviewed this
originally. I'll ping you in case you want to take it over.

Thanks, Daniel

> 
> Regards,
> 
> Tvrtko
> 
> > >   intel_overlay_last_flip_retire(struct i915_active *active)
> > >   {
> > >   struct intel_overlay *overlay =
> > > -- 
> > > 2.30.2
> > > 
> > > ___
> > > dri-devel mailing list
> > > dri-de...@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Be more gentle with exiting non-persistent context

On Thu, Apr 29, 2021 at 10:46:40AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> When a non-persistent context exits we currently mark it as banned in
> order to trigger fast termination of any outstanding GPU jobs it may have
> left running.
> 
> In doing so we apply a very strict 1ms limit in which the left over job
> has to preempt before we issues an engine resets.
> 
> Some workloads are not able to cleanly preempt in that time window and it
> can be argued that it would instead be better to give them a bit more
> grace since avoiding engine resets is generally preferrable.

Can you pls explain here why this is preferrable?

> To achieve this the patch splits handling of banned contexts from simply
> exited non-persistent ones and then applies different timeouts for both
> and also extends the criteria which determines if a request should be
> scheduled back in after preemption or not.
> 
> 15ms preempt timeout grace is given to exited non-persistent contexts
> which have been empirically tested to satisfy customers requirements
> and still provides reasonably quick cleanup post exit.

Same here, a bit more detail on what exactly was the problem to be fixed
is needed.
-Daniel

> 
> Signed-off-by: Tvrtko Ursulin 
> Cc: Chris Wilson 
> Cc: Zhen Han 
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_context.c | 15 +--
>  drivers/gpu/drm/i915/gt/intel_context.h | 17 +
>  drivers/gpu/drm/i915/gt/intel_context_types.h   |  1 +
>  .../drm/i915/gt/intel_execlists_submission.c| 12 ++--
>  drivers/gpu/drm/i915/i915_request.c |  2 +-
>  5 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index fd8ee52e17a4..5a6eba1232cd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -426,7 +426,8 @@ static struct intel_engine_cs *active_engine(struct 
> intel_context *ce)
>   return engine;
>  }
>  
> -static void kill_engines(struct i915_gem_engines *engines, bool ban)
> +static void
> +kill_engines(struct i915_gem_engines *engines, bool ban, bool persistent)
>  {
>   struct i915_gem_engines_iter it;
>   struct intel_context *ce;
> @@ -443,6 +444,8 @@ static void kill_engines(struct i915_gem_engines 
> *engines, bool ban)
>  
>   if (ban && intel_context_set_banned(ce))
>   continue;
> + else if (!persistent && intel_context_set_non_persistent(ce))
> + continue;
>  
>   /*
>* Check the current active state of this context; if we
> @@ -454,7 +457,7 @@ static void kill_engines(struct i915_gem_engines 
> *engines, bool ban)
>   engine = active_engine(ce);
>  
>   /* First attempt to gracefully cancel the context */
> - if (engine && !__cancel_engine(engine) && ban)
> + if (engine && !__cancel_engine(engine) && (ban || !persistent))
>   /*
>* If we are unable to send a preemptive pulse to bump
>* the context from the GPU, we have to resort to a full
> @@ -466,8 +469,6 @@ static void kill_engines(struct i915_gem_engines 
> *engines, bool ban)
>  
>  static void kill_context(struct i915_gem_context *ctx)
>  {
> - bool ban = (!i915_gem_context_is_persistent(ctx) ||
> - !ctx->i915->params.enable_hangcheck);
>   struct i915_gem_engines *pos, *next;
>  
>   spin_lock_irq(>stale.lock);
> @@ -480,7 +481,8 @@ static void kill_context(struct i915_gem_context *ctx)
>  
>   spin_unlock_irq(>stale.lock);
>  
> - kill_engines(pos, ban);
> + kill_engines(pos, !ctx->i915->params.enable_hangcheck,
> +  i915_gem_context_is_persistent(ctx));
>  
>   spin_lock_irq(>stale.lock);
>   GEM_BUG_ON(i915_sw_fence_signaled(>fence));
> @@ -526,7 +528,8 @@ static void engines_idle_release(struct i915_gem_context 
> *ctx,
>  
>  kill:
>   if (list_empty(>link)) /* raced, already closed */
> - kill_engines(engines, true);
> + kill_engines(engines, true,
> +  i915_gem_context_is_persistent(ctx));
>  
>   i915_sw_fence_commit(>fence);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h 
> b/drivers/gpu/drm/i915/gt/intel_context.h
> index f83a73a2b39f..b0e812b8ce39 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -220,6 +220,23 @@ static inline bool intel_context_set_banned(struct 
> intel_context *ce)
>   return test_and_set_bit(CONTEXT_BANNED, >flags);
>  }
>  
> +static inline bool intel_context_is_non_persistent(const struct 
> intel_context *ce)
> +{
> + return test_bit(CONTEXT_NON_PERSISTENT, >flags);
> +}
> +
> +static inline bool

Re: [Intel-gfx] [PATCH 09/21] drm/i915/gem: Disallow creating contexts with too many engines




On 29/04/2021 20:16, Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 3:01 AM Tvrtko Ursulin
 wrote:

On 28/04/2021 18:09, Jason Ekstrand wrote:

On Wed, Apr 28, 2021 at 9:26 AM Tvrtko Ursulin
 wrote:

On 28/04/2021 15:02, Daniel Vetter wrote:

On Wed, Apr 28, 2021 at 11:42:31AM +0100, Tvrtko Ursulin wrote:


On 28/04/2021 11:16, Daniel Vetter wrote:

On Fri, Apr 23, 2021 at 05:31:19PM -0500, Jason Ekstrand wrote:

There's no sense in allowing userspace to create more engines than it
can possibly access via execbuf.

Signed-off-by: Jason Ekstrand 
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 5f8d0faf783aa..ecb3bf5369857 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1640,11 +1640,10 @@ set_engines(struct i915_gem_context *ctx,
 return -EINVAL;
 }
-  /*
-   * Note that I915_EXEC_RING_MASK limits execbuf to only using the
-   * first 64 engines defined here.
-   */
 num_engines = (args->size - sizeof(*user)) / 
sizeof(*user->engines);


Maybe add a comment like /* RING_MASK has not shift, so can be used
directly here */ since I had to check that :-)

Same story about igt testcases needed, just to be sure.

Reviewed-by: Daniel Vetter 


I am not sure about the churn vs benefit ratio here. There are also patches
which extend the engine selection field in execbuf2 over the unused
constants bits (with an explicit flag). So churn upstream and churn in
internal (if interesting) for not much benefit.


This isn't churn.

This is "lock done uapi properly".


Pretty much.


Still haven't heard what concrete problems it solves.


IMO it is a "meh" patch. Doesn't fix any problems and will create work
for other people and man hours spent which no one will ever properly
account against.

Number of contexts in the engine map should not really be tied to
execbuf2. As is demonstrated by the incoming work to address more than
63 engines, either as an extension to execbuf2 or future execbuf3.


Which userspace driver has requested more than 64 engines in a single context?


No need to artificially limit hardware capabilities in the uapi by
implementing a policy in the kernel. Which will need to be
removed/changed shortly anyway. This particular patch is work and
creates more work (which other people who will get to fix the fallout
will spend man hours to figure out what and why broke) for no benefit.
Or you are yet to explain what the benefit is in concrete terms.


You keep complaining about how much work it takes and yet I've spent
more time replying to your e-mails on this patch than I spent writing
the patch and the IGT test.  Also, if it takes so much time to add a
restriction, then why are we spending time figuring out how to modify
the uAPI to allow you to execbuf on a context with more than 64
engines?  If we're worried about engineering man-hours, then limiting
to 64 IS the pragmatic solution.


a)

Question of what problem does the patch fix is still unanswered.

b)

You miss the point. I'll continue in the next paragraph..




Why don't you limit it to number of physical engines then? Why don't you
filter out duplicates? Why not limit the number of buffer objects per
client or global based on available RAM + swap relative to minimum
object size? Reductio ad absurdum yes, but illustrating the, in this
case, a thin line between "locking down uapi" and adding too much policy
where it is not appropriate.


All this patch does is say that  you're not allowed to create a
context with more engines than the execbuf API will let you use.  We
already have an artificial limit.  All this does is push the error
handling further up the stack.  If someone comes up with a mechanism
to execbuf on engine 65 (they'd better have an open-source user if it
involves changing API), I'm very happy for them to bump this limit at
the same time.  It'll take them 5 minutes and it'll be something they
find while writing the IGT test.


.. no it won't take five minutes.

If I need to spell everything out - you will put this patch in, which 
fixes nothing, and it will propagate to the internal kernel at some 
point. Then a bunch of tests will start failing in a strange manner. 
Which will result in people triaging them, then assigning them, then 
reserving machines, setting them up, running the repro, then digging 
into the code, and eventually figuring out what happened.


It will take hours not five minutes. And there will likely be multiple 
bug reports which most likely won't be joined so mutliple people will be 
doing multi hour debug. All for nothing. So it is rather uninteresting 
how small the change is. Interesting part is how much pointless effort 
it will create across the organisation.


Of course you may not care that much about that side of

Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

On Fri, Apr 30, 2021 at 1:28 PM Hans de Goede  wrote:
>
> Hi,
>
> On 4/29/21 9:09 PM, Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 4/29/21 2:04 PM, Daniel Vetter wrote:
> >>> On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
>  On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
> >> Userspace could hold open a reference to the connector->kdev device,
> >> through e.g. holding a sysfs-atrtribute open after
> >> drm_sysfs_connector_remove() has been called. In this case the 
> >> connector
> >> could be free-ed while the connector->kdev device's drvdata is still
> >> pointing to it.
> >>
> >> Give drm_connector devices there own device type, which allows
> >> us to specify our own release function and make 
> >> drm_sysfs_connector_add()
> >> take a reference on the connector object, and have the new release
> >> function put the reference when the device is released.
> >>
> >> Giving drm_connector devices there own device type, will also allow
> >> checking if a device is a drm_connector device with a
> >> "if (device->type == _sysfs_device_connector)" check.
> >>
> >> Note that the setting of the name member of the device_type struct will
> >> cause udev events for drm_connector-s to now contain 
> >> DEVTYPE=drm_connector
> >> as extra info. So this extends the uevent part of the userspace API.
> >>
> >> Signed-off-by: Hans de Goede 
> >
> > Are you sure? I thought sysfs is supposed to flush out any pending
> > operations (they complete fast) and handle open fd internally?
> 
>  Yes, it "should" :)
> >>>
> >>> Thanks for confirming my vague memories :-)
> >>>
> >>> Hans, pls drop this one.
> >>
> >> Please see my earlier reply to your review of this patch, it is
> >> still needed but for a different reason:
> >>
> >> """
> >> We still need this change though to make sure that the
> >> "drm/connector: Add drm_connector_find_by_fwnode() function"
> >> does not end up following a dangling drvdat pointer from one
> >> if the drm_connector kdev-s.
> >>
> >> The class_dev_iter_init() in drm_connector_find_by_fwnode() gets
> >> a reference on all devices and between getting that reference
> >> and it calling drm_connector_get() - drm_connector_unregister()
> >> may run and drop the possibly last reference to the
> >> drm_connector object, freeing it and leaving the kdev's
> >> drvdata as a dangling pointer.
> >> """
> >>
> >> This is actually why I added it initially, and while adding it
> >> I came up with this wrong theory of why it was necessary independently
> >> of the drm_connector_find_by_fwnode() addition, sorry about that.
> >
> > Generally that's handled by a kref_get_unless_zero under the protection of
> > the lock which protects the weak reference. Which I think is the right
> > model here (at a glance at least) since this is a lookup function.
>
> I'm afraid that things are a bit more complicated here. The idea here
> is that we have a subsystem outside of the DRM subsystem which received
> a hotplug event for a drm-connector.  The only info which this subsystem
> has is a reference on the fwnode level (either through device-tree or
> to platform-code instantiating software-fwnode-s + links for this).
>
> So in order to deliver the hotplug event to the connector we need
> to lookup the connector by fwnode.
>
> I've chosen to implement this by iterating over all drm_class
> devices with a dev_type of drm_connector using class_dev_iter_init()
> and friends. This makes sure that we either get a reference to
> the device, or that we skip the device if it is being deleted.
>
> But this just gives us a reference to the connector->kdev, not
> to the connector itself. A pointer to the connector itself is stored
> as drvdata inside the device, but without taking a reference as
> this patch does, there is no guarantee that that pointer does not
> point to possibly free-ed mem.
>
> We could set drvdata to 0 from drm_sysfs_connector_remove()
> Before calling device_unregister(connector->kdev) and then do
> something like this inside drm_connector_find_by_fwnode():
>
> /*
>  * Lock the device to ensure we either see the drvdata == NULL
>  * set by drm_sysfs_connector_remove(); or we block the removal
>  * from continuing until we are done with the device.
>  */
> device_lock(dev);
> connector = dev_get_drvdata(dev);
> if (connector && connector->fwnode == fwnode) {
> drm_connector_get(connector);
> found = connector;
> }
> device_unlock(dev);

Yes this is what I mean. Except not a drm_connector_get, but a
kref_get_unless_zero. The connector might already be on it's way out,
but the drvdata not yet cleared.

> With the device_lock() synchronizing against the device_lock()
> in device_unregister(connector->kdev). So that

Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector

2021-04-30 Thread Hans de Goede

Hi,

On 4/29/21 9:09 PM, Daniel Vetter wrote:
> On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 4/29/21 2:04 PM, Daniel Vetter wrote:
>>> On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
 On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
>> Userspace could hold open a reference to the connector->kdev device,
>> through e.g. holding a sysfs-atrtribute open after
>> drm_sysfs_connector_remove() has been called. In this case the connector
>> could be free-ed while the connector->kdev device's drvdata is still
>> pointing to it.
>>
>> Give drm_connector devices there own device type, which allows
>> us to specify our own release function and make drm_sysfs_connector_add()
>> take a reference on the connector object, and have the new release
>> function put the reference when the device is released.
>>
>> Giving drm_connector devices there own device type, will also allow
>> checking if a device is a drm_connector device with a
>> "if (device->type == _sysfs_device_connector)" check.
>>
>> Note that the setting of the name member of the device_type struct will
>> cause udev events for drm_connector-s to now contain 
>> DEVTYPE=drm_connector
>> as extra info. So this extends the uevent part of the userspace API.
>>
>> Signed-off-by: Hans de Goede 
>
> Are you sure? I thought sysfs is supposed to flush out any pending
> operations (they complete fast) and handle open fd internally?

 Yes, it "should" :)
>>>
>>> Thanks for confirming my vague memories :-)
>>>
>>> Hans, pls drop this one.
>>
>> Please see my earlier reply to your review of this patch, it is
>> still needed but for a different reason:
>>
>> """
>> We still need this change though to make sure that the 
>> "drm/connector: Add drm_connector_find_by_fwnode() function"
>> does not end up following a dangling drvdat pointer from one
>> if the drm_connector kdev-s.
>>
>> The class_dev_iter_init() in drm_connector_find_by_fwnode() gets
>> a reference on all devices and between getting that reference
>> and it calling drm_connector_get() - drm_connector_unregister()
>> may run and drop the possibly last reference to the
>> drm_connector object, freeing it and leaving the kdev's
>> drvdata as a dangling pointer.
>> """
>>
>> This is actually why I added it initially, and while adding it
>> I came up with this wrong theory of why it was necessary independently
>> of the drm_connector_find_by_fwnode() addition, sorry about that.
> 
> Generally that's handled by a kref_get_unless_zero under the protection of
> the lock which protects the weak reference. Which I think is the right
> model here (at a glance at least) since this is a lookup function.

I'm afraid that things are a bit more complicated here. The idea here
is that we have a subsystem outside of the DRM subsystem which received
a hotplug event for a drm-connector.  The only info which this subsystem
has is a reference on the fwnode level (either through device-tree or
to platform-code instantiating software-fwnode-s + links for this).

So in order to deliver the hotplug event to the connector we need
to lookup the connector by fwnode.

I've chosen to implement this by iterating over all drm_class
devices with a dev_type of drm_connector using class_dev_iter_init()
and friends. This makes sure that we either get a reference to
the device, or that we skip the device if it is being deleted.

But this just gives us a reference to the connector->kdev, not
to the connector itself. A pointer to the connector itself is stored
as drvdata inside the device, but without taking a reference as
this patch does, there is no guarantee that that pointer does not
point to possibly free-ed mem.

We could set drvdata to 0 from drm_sysfs_connector_remove()
Before calling device_unregister(connector->kdev) and then do
something like this inside drm_connector_find_by_fwnode():

/*
 * Lock the device to ensure we either see the drvdata == NULL
 * set by drm_sysfs_connector_remove(); or we block the removal
 * from continuing until we are done with the device.
 */
device_lock(dev);
connector = dev_get_drvdata(dev);
if (connector && connector->fwnode == fwnode) {
drm_connector_get(connector);
found = connector;
}
device_unlock(dev);

With the device_lock() synchronizing against the device_lock()
in device_unregister(connector->kdev). So that we either see
drvdata == NULL if we race with unregistering; or we get
a reference on the drm_connector obj before its ref-count can
drop to 0.

There might be places though where we call code take the device_lock
while holding a lock necessary for the drm_connector_get() , so
this approach might lead to an AB BA deadlock. As such I think
my original approach is better (also see below).

> Lookup tables holding full references tends to

Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: Use correct downstream caps for check Src-Ctl mode for PCON

2021-04-30 Thread Nautiyal, Ankit K

Hi Lakshmi,

The given possible regression is an existing issue : 
https://gitlab.freedesktop.org/drm/intel/-/issues/2867
Possible regressions

  *   igt@gem_ctx_isolation@preservation-s3@vecs0:
 *   shard-iclb: 
PASS
 -> 
DMESG-WARN
Perhaps we need to modify the filter.

Regards,
Ankit
From: Patchwork 
Sent: Thursday, April 29, 2021 10:27 PM
To: Nautiyal, Ankit K 
Cc: intel-gfx@lists.freedesktop.org
Subject: ✗ Fi.CI.IGT: failure for drm/i915: Use correct downstream caps for 
check Src-Ctl mode for PCON

Patch Details
Series:

drm/i915: Use correct downstream caps for check Src-Ctl mode for PCON

URL:

https://patchwork.freedesktop.org/series/89639/

State:

failure

Details:

https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20030/index.html

CI Bug Log - changes from CI_DRM_10027_full -> Patchwork_20030_full
Summary

FAILURE

Serious unknown changes coming with Patchwork_20030_full absolutely need to be
verified manually.

If you think the reported changes have nothing to do with the changes
introduced in Patchwork_20030_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.

Possible new issues

Here are the unknown changes that may have been introduced in 
Patchwork_20030_full:

IGT changes
Possible regressions

  *   igt@gem_ctx_isolation@preservation-s3@vecs0:
 *   shard-iclb: 
PASS
 -> 
DMESG-WARN

Known issues

Here are the changes found in Patchwork_20030_full that come from known issues:

IGT changes
Issues hit

  *   igt@gem_create@create-clear:
 *   shard-glk: 
PASS
 -> 
FAIL
 ([i915#1888] / [i915#3160])
  *   igt@gem_ctx_persistence@legacy-engines-hostile@blt:
 *   shard-tglb: 
PASS
 -> 
FAIL
 ([i915#2410])
 *   shard-kbl: 
PASS
 -> 
FAIL
 ([i915#2410])
  *   igt@gem_ctx_persistence@legacy-engines-queued:
 *   shard-snb: NOTRUN -> 
SKIP
 (fdo#109271 / 
[i915#1099]) +1 similar issue
  *   igt@gem_ctx_ringsize@active@bcs0:
 *   shard-skl: NOTRUN -> 
INCOMPLETE
 ([i915#3316])
  *   igt@gem_exec_fair@basic-deadline:
 *   shard-glk: 
PASS
 -> 
FAIL
 ([i915#2846])
  *   igt@gem_exec_fair@basic-pace-share@rcs0:
 *   shard-glk: 
PASS
 -> 
FAIL
 ([i915#2842])
  *   igt@gem_exec_fair@basic-pace@vcs1:
 *   shard-iclb: 
PASS
 -> 
FAIL
 ([i915#2842]) +2 similar issues
  *   igt@gem_mmap_gtt@cpuset-big-copy-odd:
 *   shard-glk: 
PASS
 -> 
FAIL
 ([i915#307]) +1 similar issue
  *   igt@gem_userptr_blits@set-cache-level:
 *   shard-apl: NOTRUN -> 
FAIL
 ([i915#3324])
  *   igt@gem_userptr_blits@vma-merge:

Re: [Intel-gfx] [PATCH 03/21] drm/i915/gem: Set the watchdog timeout directly in intel_context_set_gem




On 29/04/2021 15:54, Jason Ekstrand wrote:

On Thu, Apr 29, 2021 at 3:04 AM Tvrtko Ursulin
 wrote:



On 28/04/2021 18:24, Jason Ekstrand wrote:

On Wed, Apr 28, 2021 at 10:55 AM Tvrtko Ursulin
 wrote:

On 23/04/2021 23:31, Jason Ekstrand wrote:

Instead of handling it like a context param, unconditionally set it when
intel_contexts are created.  This doesn't fix anything but does simplify
the code a bit.

Signed-off-by: Jason Ekstrand 
---
drivers/gpu/drm/i915/gem/i915_gem_context.c   | 43 +++
.../gpu/drm/i915/gem/i915_gem_context_types.h |  4 --
drivers/gpu/drm/i915/gt/intel_context_param.h |  3 +-
3 files changed, 6 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 35bcdeddfbf3f..1091cc04a242a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -233,7 +233,11 @@ static void intel_context_set_gem(struct intel_context *ce,
intel_engine_has_timeslices(ce->engine))
__set_bit(CONTEXT_USE_SEMAPHORES, >flags);

- intel_context_set_watchdog_us(ce, ctx->watchdog.timeout_us);
+ if (IS_ACTIVE(CONFIG_DRM_I915_REQUEST_TIMEOUT) &&
+ ctx->i915->params.request_timeout_ms) {
+ unsigned int timeout_ms = ctx->i915->params.request_timeout_ms;
+ intel_context_set_watchdog_us(ce, (u64)timeout_ms * 1000);


Blank line between declarations and code please, or just lose the local.

Otherwise looks okay. Slight change that same GEM context can now have a
mix of different request expirations isn't interesting I think. At least
the change goes away by the end of the series.


In order for that to happen, I think you'd have to have a race between
CREATE_CONTEXT and someone smashing the request_timeout_ms param via
sysfs.  Or am I missing something?  Given that timeouts are really
per-engine anyway, I don't think we need to care too much about that.


We don't care, no.

For completeness only - by the end of the series it is what you say. But
at _this_ point in the series though it is if modparam changes at any
point between context create and replacing engines. Which is a change
compared to before this patch, since modparam was cached in the GEM
context so far. So one GEM context was a single request_timeout_ms.


I've added the following to the commit message:

It also means that sync files exported from different engines on a
SINGLE_TIMELINE context will have different fence contexts.  This is
visible to userspace if it looks at the obj_name field of
sync_fence_info.

How's that sound?


Wrong thread but sounds good.

I haven't looked into the fence merge logic apart from noticing context 
is used there. So I'd suggest a quick look there on top, just to make 
sure merging logic does not hold any surprises if contexts start to 
differ. Probably just results with more inefficiency somewhere, in theory.


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Remove erroneous i915_is_ggtt check for I915_GEM_OBJECT_UNBIND_VM_TRYLOCK

2021-04-30 Thread Maarten Lankhorst

Op 29-04-2021 om 14:01 schreef Maarten Lankhorst:
> We changed the locking hierarchy for both ppgtt and ggtt, so both locks
> should be trylocked inside i915_gem_object_unbind().
>
> Signed-off-by: Maarten Lankhorst 
> Fixes: bc6f80cce9ae ("drm/i915: Use trylock in shrinker for ggtt on bsw vt-d 
> and bxt, v2.")
> Cc: Thomas Hellström 
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 07da6a9342e3..d0018c5f88bd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -158,8 +158,7 @@ int i915_gem_object_unbind(struct drm_i915_gem_object 
> *obj,
>   ret = -EBUSY;
>   if (flags & I915_GEM_OBJECT_UNBIND_ACTIVE ||
>   !i915_vma_is_active(vma)) {
> - if (i915_is_ggtt(vma->vm) &&
> - flags & I915_GEM_OBJECT_UNBIND_VM_TRYLOCK) {
> + if (flags & I915_GEM_OBJECT_UNBIND_VM_TRYLOCK) {
>   if (mutex_trylock(>vm->mutex)) {
>   ret = __i915_vma_unbind(vma);
>   mutex_unlock(>vm->mutex);

Applied with Thomas' irc r-b.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/overlay: Fix active retire callback alignment



On 29/04/2021 18:31, Ville Syrjälä wrote:

On Thu, Apr 29, 2021 at 07:31:43PM +0300, Ville Syrjälä wrote:

On Thu, Apr 29, 2021 at 09:35:29AM +0100, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

__i915_active_call annotation is required on the retire callback to ensure
correct function alignment.

Signed-off-by: Tvrtko Ursulin 
Fixes: a21ce8ad12d2 ("drm/i915/overlay: Switch to using i915_active tracking")
Cc: Chris Wilson 
Cc: Matthew Auld 
---
  drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c 
b/drivers/gpu/drm/i915/display/intel_overlay.c
index fffbde4256db..428819ba18dd 100644
--- a/drivers/gpu/drm/i915/display/intel_overlay.c
+++ b/drivers/gpu/drm/i915/display/intel_overlay.c
@@ -383,7 +383,7 @@ static void intel_overlay_off_tail(struct intel_overlay 
*overlay)
i830_overlay_clock_gating(dev_priv, true);
  }
  
-static void

+__i915_active_call static void


Am I blind or are we just packing flag bits into a pointer, passing
that to a function, and then immediately unpack the bits again in
said function? Why not just pass the flags explicitly?

Looks like you missed auto_retire()?


Ah, just saw the other patch from Stéphane.

For the series:
Reviewed-by: Ville Syrjälä 


Thanks Ville, pushed.

Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [v3,1/5] drm/connector: Create a helper to attach the hdr_output_metadata property

== Series Details ==

Series: series starting with [v3,1/5] drm/connector: Create a helper to attach 
the hdr_output_metadata property
URL   : https://patchwork.freedesktop.org/series/89690/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10032 -> Patchwork_20043


Summary
---

  **SUCCESS**

  No regressions found.

  External URL: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/index.html

Known issues


  Here are the changes found in Patchwork_20043 that come from known issues:

### IGT changes ###

 Issues hit 

  * igt@i915_selftest@live@hangcheck:
- fi-snb-2600:[PASS][1] -> [INCOMPLETE][2] ([i915#2782])
   [1]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/fi-snb-2600/igt@i915_selftest@l...@hangcheck.html
   [2]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/fi-snb-2600/igt@i915_selftest@l...@hangcheck.html

  
 Possible fixes 

  * igt@kms_chamelium@dp-crc-fast:
- fi-kbl-7500u:   [FAIL][3] ([i915#1372]) -> [PASS][4]
   [3]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10032/fi-kbl-7500u/igt@kms_chamel...@dp-crc-fast.html
   [4]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/fi-kbl-7500u/igt@kms_chamel...@dp-crc-fast.html

  
  {name}: This element is suppressed. This means it is ignored when computing
  the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1372]: https://gitlab.freedesktop.org/drm/intel/issues/1372
  [i915#2782]: https://gitlab.freedesktop.org/drm/intel/issues/2782
  [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303


Participating hosts (43 -> 38)
--

  Missing(5): fi-ilk-m540 fi-hsw-4200u fi-bsw-n3050 fi-bdw-gvtdvm 
fi-bsw-cyan 


Build changes
-

  * Linux: CI_DRM_10032 -> Patchwork_20043

  CI-20190529: 20190529
  CI_DRM_10032: dcaca2238f4a55978a4e5e0aa99cb830453c7c98 @ 
git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6077: 126a3f6fc0e97786e2819085efc84e741093aed5 @ 
git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_20043: df84548b8d57b73780b9cb6d476f21b2dec0da6a @ 
git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

df84548b8d57 drm/vc4: hdmi: Signal the proper colorimetry info in the infoframe
f576047b2a0c drm/connector: Add a helper to attach the colorspace property
4601e9361ce4 drm/vc4: Add HDR metadata property to the VC5 HDMI connectors
19ebe25f0b40 drm/connector: Add helper to compare HDR metadata
edc37a6f6280 drm/connector: Create a helper to attach the hdr_output_metadata 
property

== Logs ==

For more details see: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20043/index.html
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [v3,1/5] drm/connector: Create a helper to attach the hdr_output_metadata property

== Series Details ==

Series: series starting with [v3,1/5] drm/connector: Create a helper to attach 
the hdr_output_metadata property
URL   : https://patchwork.freedesktop.org/series/89690/
State : warning

== Summary ==

$ dim sparse --fast origin/drm-tip
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.
-
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed: "amd_sriov_msg_vf2pf_info must be 1 KB"
+./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgv_sriovmsg.h:270:49: error: static 
assertion failed:

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [v3,1/5] drm/connector: Create a helper to attach the hdr_output_metadata property

== Series Details ==

Series: series starting with [v3,1/5] drm/connector: Create a helper to attach 
the hdr_output_metadata property
URL   : https://patchwork.freedesktop.org/series/89690/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
edc37a6f6280 drm/connector: Create a helper to attach the hdr_output_metadata 
property
19ebe25f0b40 drm/connector: Add helper to compare HDR metadata
4601e9361ce4 drm/vc4: Add HDR metadata property to the VC5 HDMI connectors
-:21: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#21: FILE: drivers/gpu/drm/vc4/vc4_hdmi.c:218:
+static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state)

total: 0 errors, 0 warnings, 1 checks, 110 lines checked
f576047b2a0c drm/connector: Add a helper to attach the colorspace property
df84548b8d57 drm/vc4: hdmi: Signal the proper colorimetry info in the infoframe


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 08/21] drm/i915/gem: Disallow bonding of virtual engines

On Thu, Apr 29, 2021 at 09:03:48PM -0700, Matthew Brost wrote:
> On Thu, Apr 29, 2021 at 02:14:19PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 28, 2021 at 01:17:27PM -0500, Jason Ekstrand wrote:
> > > On Wed, Apr 28, 2021 at 1:02 PM Matthew Brost  
> > > wrote:
> > > >
> > > > On Wed, Apr 28, 2021 at 12:46:07PM -0500, Jason Ekstrand wrote:
> > > > > On Wed, Apr 28, 2021 at 12:26 PM Matthew Brost 
> > > > >  wrote:
> > > > > > Jumping on here mid-thread. For what is is worth to make execlists 
> > > > > > work
> > > > > > with the upcoming parallel submission extension I leveraged some of 
> > > > > > the
> > > > > > existing bonding code so I wouldn't be too eager to delete this code
> > > > > > until that lands.
> > > > >
> > > > > Mind being a bit more specific about that?  The motivation for this
> > > > > patch is that the current bonding handling and uAPI is, well, very odd
> > > > > and confusing IMO.  It doesn't let you create sets of bonded engines.
> > > > > Instead you create engines and then bond them together after the fact.
> > > > > I didn't want to blindly duplicate those oddities with the proto-ctx
> > > > > stuff unless they were useful.  With parallel submit, I would expect
> > > > > we want a more explicit API where you specify a set of engine
> > > > > class/instance pairs to bond together into a single engine similar to
> > > > > how the current balancing API works.
> > > > >
> > > > > Of course, that's all focused on the API and not the internals.  But,
> > > > > again, I'm not sure how we want things to look internally.  What we've
> > > > > got now doesn't seem great for the GuC submission model but I'm very
> > > > > much not the expert there.  I don't want to be working at cross
> > > > > purposes to you and I'm happy to leave bits if you think they're
> > > > > useful.  But I thought I was clearing things away so that you can put
> > > > > in what you actually want for GuC/parallel submit.
> > > > >
> > > >
> > > > Removing all the UAPI things are fine but I wouldn't delete some of the
> > > > internal stuff (e.g. intel_virtual_engine_attach_bond, bond
> > > > intel_context_ops, the hook for a submit fence, etc...) as that will
> > > > still likely be used for the new parallel submission interface with
> > > > execlists. As you say the new UAPI wont allow crazy configurations,
> > > > only simple ones.
> > > 
> > > I'm fine with leaving some of the internal bits for a little while if
> > > it makes pulling the GuC scheduler in easier.  I'm just a bit
> > > skeptical of why you'd care about SUBMIT_FENCE. :-)  Daniel, any
> > > thoughts?
> > 
> > Yeah I'm also wondering why we need this. Essentially your insight (and
> > Tony Ye from media team confirmed) is that media umd never uses bonded on
> > virtual engines.
> >
> 
> Well you should use virtual engines with parallel submission interface 
> if are you using it correctly.
> 
> e.g. You want a 2 wide parallel submission and there are 4 engine
> instances.
> 
> You'd create 2 VEs:
> 
> A: 0, 2
> B: 1, 3
> set_parallel

So tbh I'm not really liking this part. At least my understanding is that
with GuC this is really one overall virtual engine, backed by a multi-lrc.

So it should fill one engine slot, not fill multiple virtual engines and
then be an awkward thing wrapped on top.

I think (but maybe my understanding of GuC and the parallel submit execbuf
interface is wrong) that the parallel engine should occupy a single VE
slot, not require additional VE just for fun (maybe the execlist backend
would require that internally, but that should not leak into the higher
levels, much less the uapi). And you submit your multi-batch execbuf on
that single parallel VE, which then gets passed to GuC as a multi-LRC.
Internally in the backend there's a bit of fan-out to put the right
MI_BB_START into the right rings and all that, but again I think that
should be backend concerns.

Or am I missing something big here?

> For GuC submission we just configure context and the GuC load balances
> it.
> 
> For execlists we'd need to create bonds.
> 
> Also likely the reason virtual engines wasn't used with the old
> interface was we only had 2 instances max per class so no need for
> virtual engines. If they used it for my above example if they were using
> the interface correctly they would have to use virtual engines too.

They do actually use virtual engines, it's just the virtual engine only
contains a single one, and internally i915 folds that into the hw engine
directly. So we can take away the entire implementation complexity.

Also I still think for execlist we shouldn't bother with trying to enable
parallel submit. Or at least only way down if there's no other reasonable
option.

> > So the only thing we need is the await_fence submit_fence logic to stall
> > the subsequent patches just long enough. I think that stays.
> >
> 
> My implementation, for the new parallel submission interface, with
> execlists used a bonds + priority boosts to

[Intel-gfx] [PATCH v3 4/5] drm/connector: Add a helper to attach the colorspace property

The intel driver uses the same logic to attach the Colorspace property
in multiple places and we'll need it in vc4 too. Let's move that common
code in a helper.

Signed-off-by: Maxime Ripard 

---

Changes from v2:
  - Rebased on current drm-misc-next

Changes from v1:
  - New patch
---
 drivers/gpu/drm/drm_connector.c   | 20 +++
 .../gpu/drm/i915/display/intel_connector.c|  6 ++
 include/drm/drm_connector.h   |  1 +
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index eed9cd05c94e..733da42cfd31 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2172,6 +2172,26 @@ int 
drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
 }
 EXPORT_SYMBOL(drm_connector_attach_hdr_output_metadata_property);
 
+/**
+ * drm_connector_attach_colorspace_property - attach "Colorspace" property
+ * @connector: connector to attach the property on.
+ *
+ * This is used to allow the userspace to signal the output colorspace
+ * to the driver.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_colorspace_property(struct drm_connector *connector)
+{
+   struct drm_property *prop = connector->colorspace_property;
+
+   drm_object_attach_property(>base, prop, 
DRM_MODE_COLORIMETRY_DEFAULT);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_colorspace_property);
+
 /**
  * drm_connector_atomic_hdr_metadata_equal - checks if the hdr metadata changed
  * @old_state: old connector state to compare
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c 
b/drivers/gpu/drm/i915/display/intel_connector.c
index d5ceb7bdc14b..9bed1ccecea0 100644
--- a/drivers/gpu/drm/i915/display/intel_connector.c
+++ b/drivers/gpu/drm/i915/display/intel_connector.c
@@ -282,14 +282,12 @@ void
 intel_attach_hdmi_colorspace_property(struct drm_connector *connector)
 {
if (!drm_mode_create_hdmi_colorspace_property(connector))
-   drm_object_attach_property(>base,
-  connector->colorspace_property, 0);
+   drm_connector_attach_colorspace_property(connector);
 }
 
 void
 intel_attach_dp_colorspace_property(struct drm_connector *connector)
 {
if (!drm_mode_create_dp_colorspace_property(connector))
-   drm_object_attach_property(>base,
-  connector->colorspace_property, 0);
+   drm_connector_attach_colorspace_property(connector);
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1f51d73ca715..714d1a01c065 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1671,6 +1671,7 @@ int drm_connector_attach_scaling_mode_property(struct 
drm_connector *connector,
   u32 scaling_mode_mask);
 int drm_connector_attach_vrr_capable_property(
struct drm_connector *connector);
+int drm_connector_attach_colorspace_property(struct drm_connector *connector);
 int drm_connector_attach_hdr_output_metadata_property(struct drm_connector 
*connector);
 bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state 
*old_state,
 struct drm_connector_state 
*new_state);
-- 
2.31.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 5/5] drm/vc4: hdmi: Signal the proper colorimetry info in the infoframe

Our driver while supporting HDR didn't send the proper colorimetry info
in the AVI infoframe.

Let's add the property needed so that the userspace can let us know what
the colorspace is supposed to be.

Signed-off-by: Maxime Ripard 

---

Changes from v2:
  - Rebased on current drm-misc-next

Changes from v1:
  - New patch
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index a33fa1662588..a22e17788074 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -226,7 +226,8 @@ static int vc4_hdmi_connector_atomic_check(struct 
drm_connector *connector,
if (!crtc)
return 0;
 
-   if (!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
+   if (old_state->colorspace != new_state->colorspace ||
+   !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
struct drm_crtc_state *crtc_state;
 
crtc_state = drm_atomic_get_crtc_state(state, crtc);
@@ -316,6 +317,11 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
if (ret)
return ret;
 
+   ret = drm_mode_create_hdmi_colorspace_property(connector);
+   if (ret)
+   return ret;
+
+   drm_connector_attach_colorspace_property(connector);
drm_connector_attach_tv_margin_properties(connector);
drm_connector_attach_max_bpc_property(connector, 8, 12);
 
@@ -424,7 +430,7 @@ static void vc4_hdmi_set_avi_infoframe(struct drm_encoder 
*encoder)
   vc4_encoder->limited_rgb_range ?
   HDMI_QUANTIZATION_RANGE_LIMITED :
   HDMI_QUANTIZATION_RANGE_FULL);
-
+   drm_hdmi_avi_infoframe_colorspace(, cstate);
drm_hdmi_avi_infoframe_bars(, cstate);
 
vc4_hdmi_write_infoframe(encoder, );
-- 
2.31.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 2/5] drm/connector: Add helper to compare HDR metadata

All the drivers that support the HDR metadata property have a similar
function to compare the metadata from one connector state to the next,
and force a mode change if they differ.

All these functions run pretty much the same code, so let's turn it into
an helper that can be shared across those drivers.

Reviewed-by: Harry Wentland 
Reviewed-by: Jernej Skrabec 
Signed-off-by: Maxime Ripard 

---

Changes from v2:
  - Rebased on current drm-misc-next

Changes from v1:
  - Rebased on latest drm-misc-next tag
  - Added the tags
  - Fix build breakage on amdgpu
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++-
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +--
 drivers/gpu/drm/drm_connector.c   | 28 +++
 drivers/gpu/drm/i915/display/intel_atomic.c   | 13 +
 include/drm/drm_connector.h   |  2 ++
 5 files changed, 34 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index c8d7e7dbc05e..296704ce3768 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6275,25 +6275,6 @@ static int fill_hdr_info_packet(const struct 
drm_connector_state *state,
return 0;
 }
 
-static bool
-is_hdr_metadata_different(const struct drm_connector_state *old_state,
- const struct drm_connector_state *new_state)
-{
-   struct drm_property_blob *old_blob = old_state->hdr_output_metadata;
-   struct drm_property_blob *new_blob = new_state->hdr_output_metadata;
-
-   if (old_blob != new_blob) {
-   if (old_blob && new_blob &&
-   old_blob->length == new_blob->length)
-   return memcmp(old_blob->data, new_blob->data,
- old_blob->length);
-
-   return true;
-   }
-
-   return false;
-}
-
 static int
 amdgpu_dm_connector_atomic_check(struct drm_connector *conn,
 struct drm_atomic_state *state)
@@ -6311,7 +6292,7 @@ amdgpu_dm_connector_atomic_check(struct drm_connector 
*conn,
if (!crtc)
return 0;
 
-   if (is_hdr_metadata_different(old_con_state, new_con_state)) {
+   if (!drm_connector_atomic_hdr_metadata_equal(old_con_state, 
new_con_state)) {
struct dc_info_packet hdr_infopacket;
 
ret = fill_hdr_info_packet(new_con_state, _infopacket);
@@ -8803,7 +8784,7 @@ static void amdgpu_dm_atomic_commit_tail(struct 
drm_atomic_state *state)
  dm_old_crtc_state->abm_level;
 
hdr_changed =
-   is_hdr_metadata_different(old_con_state, new_con_state);
+   !drm_connector_atomic_hdr_metadata_equal(old_con_state, 
new_con_state);
 
if (!scaling_changed && !abm_changed && !hdr_changed)
continue;
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index dd7f6eda2ce2..e7c7c9b9c646 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2395,21 +2395,6 @@ static int dw_hdmi_connector_get_modes(struct 
drm_connector *connector)
return ret;
 }
 
-static bool hdr_metadata_equal(const struct drm_connector_state *old_state,
-  const struct drm_connector_state *new_state)
-{
-   struct drm_property_blob *old_blob = old_state->hdr_output_metadata;
-   struct drm_property_blob *new_blob = new_state->hdr_output_metadata;
-
-   if (!old_blob || !new_blob)
-   return old_blob == new_blob;
-
-   if (old_blob->length != new_blob->length)
-   return false;
-
-   return !memcmp(old_blob->data, new_blob->data, old_blob->length);
-}
-
 static int dw_hdmi_connector_atomic_check(struct drm_connector *connector,
  struct drm_atomic_state *state)
 {
@@ -2423,7 +2408,7 @@ static int dw_hdmi_connector_atomic_check(struct 
drm_connector *connector,
if (!crtc)
return 0;
 
-   if (!hdr_metadata_equal(old_state, new_state)) {
+   if (!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
crtc_state = drm_atomic_get_crtc_state(state, crtc);
if (IS_ERR(crtc_state))
return PTR_ERR(crtc_state);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index c5e2f642acd9..eed9cd05c94e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2172,6 +2172,34 @@ int 
drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn
 }
 EXPORT_SYMBOL(drm_connector_attach_hdr_output_metadata_property);
 
+/**
+ * drm_connector_atomic_hdr_metadata_equal - checks if the hdr metadata changed
+ * @old_state: old

[Intel-gfx] [PATCH v3 3/5] drm/vc4: Add HDR metadata property to the VC5 HDMI connectors

From: Dave Stevenson 

Now that we can export deeper colour depths, add in the signalling
for HDR metadata.

Signed-off-by: Dave Stevenson 
Signed-off-by: Maxime Ripard 

---

Changes from v2:
  - Rebased on current drm-misc-next

Changes from v1:
  - Rebased on latest drm-misc-next tag
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 53 ++
 drivers/gpu/drm/vc4/vc4_hdmi.h |  3 ++
 2 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 1fda574579af..a33fa1662588 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -214,6 +214,31 @@ static int vc4_hdmi_connector_get_modes(struct 
drm_connector *connector)
return ret;
 }
 
+static int vc4_hdmi_connector_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state)
+{
+   struct drm_connector_state *old_state =
+   drm_atomic_get_old_connector_state(state, connector);
+   struct drm_connector_state *new_state =
+   drm_atomic_get_new_connector_state(state, connector);
+   struct drm_crtc *crtc = new_state->crtc;
+
+   if (!crtc)
+   return 0;
+
+   if (!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) {
+   struct drm_crtc_state *crtc_state;
+
+   crtc_state = drm_atomic_get_crtc_state(state, crtc);
+   if (IS_ERR(crtc_state))
+   return PTR_ERR(crtc_state);
+
+   crtc_state->mode_changed = true;
+   }
+
+   return 0;
+}
+
 static void vc4_hdmi_connector_reset(struct drm_connector *connector)
 {
struct vc4_hdmi_connector_state *old_state =
@@ -263,6 +288,7 @@ static const struct drm_connector_funcs 
vc4_hdmi_connector_funcs = {
 
 static const struct drm_connector_helper_funcs vc4_hdmi_connector_helper_funcs 
= {
.get_modes = vc4_hdmi_connector_get_modes,
+   .atomic_check = vc4_hdmi_connector_atomic_check,
 };
 
 static int vc4_hdmi_connector_init(struct drm_device *dev,
@@ -299,6 +325,9 @@ static int vc4_hdmi_connector_init(struct drm_device *dev,
connector->interlace_allowed = 1;
connector->doublescan_allowed = 0;
 
+   if (vc4_hdmi->variant->supports_hdr)
+   drm_connector_attach_hdr_output_metadata_property(connector);
+
drm_connector_attach_encoder(connector, encoder);
 
return 0;
@@ -432,6 +461,25 @@ static void vc4_hdmi_set_audio_infoframe(struct 
drm_encoder *encoder)
vc4_hdmi_write_infoframe(encoder, );
 }
 
+static void vc4_hdmi_set_hdr_infoframe(struct drm_encoder *encoder)
+{
+   struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
+   struct drm_connector *connector = _hdmi->connector;
+   struct drm_connector_state *conn_state = connector->state;
+   union hdmi_infoframe frame;
+
+   if (!vc4_hdmi->variant->supports_hdr)
+   return;
+
+   if (!conn_state->hdr_output_metadata)
+   return;
+
+   if (drm_hdmi_infoframe_set_hdr_metadata(, conn_state))
+   return;
+
+   vc4_hdmi_write_infoframe(encoder, );
+}
+
 static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
 {
struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
@@ -444,6 +492,8 @@ static void vc4_hdmi_set_infoframes(struct drm_encoder 
*encoder)
 */
if (vc4_hdmi->audio.streaming)
vc4_hdmi_set_audio_infoframe(encoder);
+
+   vc4_hdmi_set_hdr_infoframe(encoder);
 }
 
 static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder,
@@ -2102,6 +2152,7 @@ static const struct vc4_hdmi_variant bcm2835_variant = {
.phy_rng_enable = vc4_hdmi_phy_rng_enable,
.phy_rng_disable= vc4_hdmi_phy_rng_disable,
.channel_map= vc4_hdmi_channel_map,
+   .supports_hdr   = false,
 };
 
 static const struct vc4_hdmi_variant bcm2711_hdmi0_variant = {
@@ -2129,6 +2180,7 @@ static const struct vc4_hdmi_variant 
bcm2711_hdmi0_variant = {
.phy_rng_enable = vc5_hdmi_phy_rng_enable,
.phy_rng_disable= vc5_hdmi_phy_rng_disable,
.channel_map= vc5_hdmi_channel_map,
+   .supports_hdr   = true,
 };
 
 static const struct vc4_hdmi_variant bcm2711_hdmi1_variant = {
@@ -2156,6 +2208,7 @@ static const struct vc4_hdmi_variant 
bcm2711_hdmi1_variant = {
.phy_rng_enable = vc5_hdmi_phy_rng_enable,
.phy_rng_disable= vc5_hdmi_phy_rng_disable,
.channel_map= vc5_hdmi_channel_map,
+   .supports_hdr   = true,
 };
 
 static const struct of_device_id vc4_hdmi_dt_match[] = {
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 3cebd1fd00fc..060bcaefbeb5 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -99,6 +99,9 @@ struct vc4_hdmi_variant {
 
/*

[Intel-gfx] [PATCH v3 1/5] drm/connector: Create a helper to attach the hdr_output_metadata property

All the drivers that implement HDR output call pretty much the same
function to initialise the hdr_output_metadata property, and while the
creation of that property is in a helper, every driver uses the same
code to attach it.

Provide a helper for it as well

Reviewed-by: Harry Wentland 
Reviewed-by: Jernej Skrabec 
Signed-off-by: Maxime Ripard 

---

Changes from v2:
  - Rebased on current drm-misc-next
  - Fixed a merge conflict with i915

Changes from v1:
  - Rebased on latest drm-misc-next tag
  - Added the tags
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 +---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  3 +--
 drivers/gpu/drm/drm_connector.c   | 21 +++
 drivers/gpu/drm/i915/display/intel_hdmi.c |  3 +--
 include/drm/drm_connector.h   |  1 +
 5 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a0c8c41e4e57..c8d7e7dbc05e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7498,9 +7498,7 @@ void amdgpu_dm_connector_init_helper(struct 
amdgpu_display_manager *dm,
if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
connector_type == DRM_MODE_CONNECTOR_eDP) {
-   drm_object_attach_property(
-   >base.base,
-   dm->ddev->mode_config.hdr_output_metadata_property, 0);
+   
drm_connector_attach_hdr_output_metadata_property(>base);
 
if (!aconnector->mst_port)

drm_connector_attach_vrr_capable_property(>base);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index ae97513ef886..dd7f6eda2ce2 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2492,8 +2492,7 @@ static int dw_hdmi_connector_create(struct dw_hdmi *hdmi)
drm_connector_attach_max_bpc_property(connector, 8, 16);
 
if (hdmi->version >= 0x200a && hdmi->plat_data->use_drm_infoframe)
-   drm_object_attach_property(>base,
-   
connector->dev->mode_config.hdr_output_metadata_property, 0);
+   drm_connector_attach_hdr_output_metadata_property(connector);
 
drm_connector_attach_encoder(connector, hdmi->bridge.encoder);
 
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index eab8c0b82de2..c5e2f642acd9 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2151,6 +2151,27 @@ int drm_connector_attach_max_bpc_property(struct 
drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_max_bpc_property);
 
+/**
+ * drm_connector_attach_hdr_output_metadata_property - attach 
"HDR_OUTPUT_METADA" property
+ * @connector: connector to attach the property on.
+ *
+ * This is used to allow the userspace to send HDR Metadata to the
+ * driver.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_hdr_output_metadata_property(struct drm_connector 
*connector)
+{
+   struct drm_device *dev = connector->dev;
+   struct drm_property *prop = 
dev->mode_config.hdr_output_metadata_property;
+
+   drm_object_attach_property(>base, prop, 0);
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_hdr_output_metadata_property);
+
 /**
  * drm_connector_set_vrr_capable_property - sets the variable refresh rate
  * capable property for a connector
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 9c172dd6fb5b..3c767bcc47b1 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2459,8 +2459,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, 
struct drm_connector *c
drm_connector_attach_content_type_property(connector);
 
if (DISPLAY_VER(dev_priv) >= 10)
-   drm_object_attach_property(>base,
-   
connector->dev->mode_config.hdr_output_metadata_property, 0);
+   drm_connector_attach_hdr_output_metadata_property(connector);
 
if (!HAS_GMCH(dev_priv))
drm_connector_attach_max_bpc_property(connector, 8, 12);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 1922b278ffad..32172dab8427 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1671,6 +1671,7 @@ int drm_connector_attach_scaling_mode_property(struct 
drm_connector *connector,
   u32 scaling_mode_mask);
 int drm_connector_attach_vrr_capable_property(
struct drm_connector *connector);
+int drm_connector_attach_hdr_output_metadata_property(struct drm_connector 
*connector);
 int

[Intel-gfx] [PULL] drm-intel-next-fixes for the merge window #2

2021-04-30 Thread Jani Nikula



Hi Dave & Daniel -

One more fix for -rc1.

drm-intel-next-fixes-2021-04-30:
drm/i915 GVT fixes for v5.13-rc1:
- Fix a possible division by zero in vgpu display rate calculation

BR,
Jani.

The following changes since commit 270e3cc5aa382f63ea20b93c3d20162a891dc638:

  drm/i915: Fix docbook descriptions for i915_gem_shrinker (2021-04-26 11:54:33 
+0300)

are available in the Git repository at:

  git://anongit.freedesktop.org/drm/drm-intel 
tags/drm-intel-next-fixes-2021-04-30

for you to fetch changes up to c7b397e9ca4d6828e3e3f504c80bcb1fe535c348:

  Merge tag 'gvt-next-fixes-2021-04-29' of https://github.com/intel/gvt-linux 
into drm-intel-next-fixes (2021-04-29 13:15:51 +0300)


drm/i915 GVT fixes for v5.13-rc1:
- Fix a possible division by zero in vgpu display rate calculation


Colin Xu (1):
  drm/i915/gvt: Prevent divided by zero when calculating refresh rate

Jani Nikula (1):
  Merge tag 'gvt-next-fixes-2021-04-29' of 
https://github.com/intel/gvt-linux into drm-intel-next-fixes

 drivers/gpu/drm/i915/gvt/handlers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] REGRESSION with 5.12: Suspend not working on Toshiba notebook

2021-04-30 Thread Joonas Lahtinen

+ intel-gfx mailing list

Quoting Zhenyu Wang (2021-04-30 02:58:28)
> On 2021.04.29 20:14:50 +0200, Andreas Friedrich wrote:
> > Hello Joonas,
> > 
> > with kernel 5.12 suspend to memory is no longer working on my old
> > Toshiba Tecra A10 notebook.
> > 
> > After calling /sbin/pm-suspend the screen is dimmed but the CPU fan is
> > still running, the system is frozen and is not in suspended state. The
> > only way to get the system working again is to hard power off and on.
> > 
> > Kernel 5.11.16 works well.
> > 
> > Bisecting the kernel leads me to your commit, which caused the problem:
> > 
> >   commit c071a6c0fef0fade787d827c7fc0e07481512326 (HEAD, refs/bisect/bad)
> >   Merge: a2dd2ff50cde 02dd2b12a685
> >   Author: Joonas Lahtinen 
> >   Date:   Thu Jan 21 15:10:16 2021 +0200
> > 
> > Merge tag 'gvt-gt-next-2021-01-18' of 
> > https://github.com/intel/gvt-linux into drm-intel-gt-next
> > 
> > gvt-gt-next-2021-01-18
> > 
> > - GVT cmd parser enhancement against guest context (Yan)
> > 
> > Signed-off-by: Joonas Lahtinen 
> > From: Zhenyu Wang 
> > Link: 
> > https://patchwork.freedesktop.org/patch/msgid/20210118050739.gy15...@zhen-hp.sh.intel.com

That is a merge commit, it doesn't itself change anything as there were no
conflicts. It just indicates that two trees got merged.

Bisecting should end up in a commit that has code changes. Then you should
be able to try to revert the offending commit from the latest mainline kernel.
Most of the time that can be used to confirm that reverting specific commit
fixes issue (unless the commit has a lot of dependencies). This is not
required though, if you are able to bisect to a single code change commit
that'll allow us to provide fix to try.

If you are able to provide a bisect to a one patch, please do report it
as a bug, let's then take it from there.

See:

https://01.org/linuxgraphics/documentation/how-report-bugs

> > Not every suspend results in a frozen system, so I think a race
> > condition is on the way.

This would further tip the scale towards the bisect being a false
positive. I know it can be painful to do dozens of runs per each
bisecting point, but it's really needed to arrive at the right patch.

What kind of rate you have for hitting the issue on 5.12 kernel?

> > Appendix:
> > - Kernel configuration "5.12.config.gz"
> > - My grub kernel boot parameters:
> >   root=/dev/sda5 snd-hda-intel.model=toshiba resume=/dev/sda3 printk.time=1
> > 
> > Please let me know if I could help to fix the problem.
> > 
> 
> Looks gvt is not enabled in config or runtime, so can't think that gvt merge 
> should be related...

Yes, I think the merge commit is a red herring.

Regards, Joonas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: Fix wrong name announced on FB driver switching

2021-04-30 Thread Janusz Krzysztofik

On piątek, 30 kwietnia 2021 01:01:38 CEST Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Fix wrong name announced on FB driver switching
> URL   : https://patchwork.freedesktop.org/series/89663/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_10027_full -> Patchwork_20039_full
> 
> 
> Summary
> ---
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with Patchwork_20039_full absolutely need to 
> be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_20039_full, please notify your bug team to allow 
> them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   
> 
> Possible new issues
> ---
> 
>   Here are the unknown changes that may have been introduced in 
> Patchwork_20039_full:
> 
> ### IGT changes ###
> 
>  Possible regressions 
> 
>   * igt@kms_plane@plane-panning-bottom-right-pipe-b-planes:
> - shard-tglb: [PASS][1] -> [DMESG-WARN][2] +4 similar issues
>[1]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/shard-tglb3/igt@kms_pl...@plane-panning-bottom-right-pipe-b-planes.html
>[2]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-tglb5/igt@kms_pl...@plane-panning-bottom-right-pipe-b-planes.html

False positive.  The change only affects a notice sent to kernel log on FB 
switching, nothing else, then there is no possibility for any error messages 
in kernel log being related.

Thanks,
Janusz

> 
>   
> Known issues
> 
> 
>   Here are the changes found in Patchwork_20039_full that come from known 
> issues:
> 
> ### IGT changes ###
> 
>  Issues hit 
> 
>   * igt@gem_create@create-clear:
> - shard-glk:  [PASS][3] -> [FAIL][4] ([i915#3160])
>[3]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/shard-glk8/igt@gem_cre...@create-clear.html
>[4]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-glk9/igt@gem_cre...@create-clear.html
> - shard-skl:  [PASS][5] -> [FAIL][6] ([i915#3160])
>[5]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/shard-skl9/igt@gem_cre...@create-clear.html
>[6]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-skl9/igt@gem_cre...@create-clear.html
> 
>   * igt@gem_create@create-massive:
> - shard-apl:  NOTRUN -> [DMESG-WARN][7] ([i915#3002])
>[7]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-apl3/igt@gem_cre...@create-massive.html
> 
>   * igt@gem_ctx_persistence@legacy-engines-hostile@render:
> - shard-iclb: [PASS][8] -> [FAIL][9] ([i915#2410])
>[8]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/shard-iclb6/igt@gem_ctx_persistence@legacy-engines-host...@render.html
>[9]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-iclb6/igt@gem_ctx_persistence@legacy-engines-host...@render.html
> 
>   * igt@gem_ctx_persistence@legacy-engines-queued:
> - shard-snb:  NOTRUN -> [SKIP][10] ([fdo#109271] / [i915#1099]) 
> +2 similar issues
>[10]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-snb5/igt@gem_ctx_persiste...@legacy-engines-queued.html
> 
>   * igt@gem_ctx_persistence@many-contexts:
> - shard-tglb: [PASS][11] -> [FAIL][12] ([i915#2410])
>[11]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/shard-tglb6/igt@gem_ctx_persiste...@many-contexts.html
>[12]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-tglb2/igt@gem_ctx_persiste...@many-contexts.html
> 
>   * igt@gem_ctx_ringsize@active@bcs0:
> - shard-skl:  NOTRUN -> [INCOMPLETE][13] ([i915#3316])
>[13]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-skl8/igt@gem_ctx_ringsize@act...@bcs0.html
> 
>   * igt@gem_exec_fair@basic-deadline:
> - shard-kbl:  [PASS][14] -> [FAIL][15] ([i915#2846])
>[14]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/shard-kbl2/igt@gem_exec_f...@basic-deadline.html
>[15]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-kbl4/igt@gem_exec_f...@basic-deadline.html
> 
>   * igt@gem_exec_fair@basic-flow@rcs0:
> - shard-kbl:  [PASS][16] -> [SKIP][17] ([fdo#109271])
>[16]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/shard-kbl7/igt@gem_exec_fair@basic-f...@rcs0.html
>[17]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-kbl6/igt@gem_exec_fair@basic-f...@rcs0.html
> 
>   * igt@gem_exec_fair@basic-pace@rcs0:
> - shard-kbl:  [PASS][18] -> [FAIL][19] ([i915#2842]) +1 similar 
> issue
>[18]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/shard-kbl7/igt@gem_exec_fair@basic-p...@rcs0.html
>[19]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20039/shard-kbl4/igt@gem_exec_fair@basic-p...@rcs0.html
> 
>   *

Re: [Intel-gfx] [PATCH v3 15/16] drm/i915/pxp: black pixels on pxp disabled

2021-04-30 Thread Gupta, Anshuman




> -Original Message-
> From: Ville Syrjälä 
> Sent: Wednesday, April 28, 2021 12:26 AM
> To: Gupta, Anshuman 
> Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo ;
> Gaurav, Kumar ; Shankar, Uma
> ; Ceraolo Spurio, Daniele
> 
> Subject: Re: [PATCH v3 15/16] drm/i915/pxp: black pixels on pxp disabled
> 
> On Tue, Apr 27, 2021 at 04:15:04PM +0530, Anshuman Gupta wrote:
> > When protected sufaces has flipped and pxp session is disabled,
> > display black pixels by using plane color CTM correction.
> >
> > v2:
> > - Display black pixels in aysnc flip too.   
> 
> We can't change any of that with an async flip.
I was thinking of an scenario , when application flip the protected surfaces 
with synchronous flips
and driver has enable the plane decryption, can application issue an 
intermediate async flip with
protected surfaces afterwards ?
If above is possible, is it possible to display black pixels in case of pxp 
session invalidation at the time of
Plane commit?   
Thanks,
Anshuman
> 
> >
> > Cc: Ville Syrjälä 
> > Cc: Gaurav Kumar 
> > Cc: Shankar Uma 
> > Signed-off-by: Anshuman Gupta 
> > Signed-off-by: Daniele Ceraolo Spurio
> > 
> > ---
> >  .../drm/i915/display/skl_universal_plane.c| 51 ++-
> >  drivers/gpu/drm/i915/i915_reg.h   | 46 +
> >  2 files changed, 95 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 74489217e580..a666b86df726 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -934,6 +934,33 @@ static u32 glk_plane_color_ctl(const struct
> intel_crtc_state *crtc_state,
> > return plane_color_ctl;
> >  }
> >
> > +static void intel_load_plane_csc_black(struct intel_plane
> > +*intel_plane) {
> > +   struct drm_i915_private *dev_priv = to_i915(intel_plane->base.dev);
> > +   enum pipe pipe = intel_plane->pipe;
> > +   enum plane_id plane = intel_plane->id;
> > +   u16 postoff = 0;
> > +
> > +   drm_dbg_kms(_priv->drm, "plane color CTM to black  %s:%d\n",
> > +   intel_plane->base.name, plane);
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_COEFF(pipe, plane, 0), 0);
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_COEFF(pipe, plane, 1), 0);
> > +
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_COEFF(pipe, plane, 2), 0);
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_COEFF(pipe, plane, 3), 0);
> > +
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_COEFF(pipe, plane, 4), 0);
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_COEFF(pipe, plane, 5), 0);
> > +
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_PREOFF(pipe, plane, 0), 0);
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_PREOFF(pipe, plane, 1), 0);
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_PREOFF(pipe, plane, 2), 0);
> > +
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_POSTOFF(pipe, plane, 0),
> postoff);
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_POSTOFF(pipe, plane, 1),
> postoff);
> > +   intel_de_write_fw(dev_priv, PLANE_CSC_POSTOFF(pipe, plane, 2),
> > +postoff); }
> > +
> >  static void
> >  skl_program_plane(struct intel_plane *plane,
> >   const struct intel_crtc_state *crtc_state, @@ -1039,13
> +1066,22
> > @@ skl_program_plane(struct intel_plane *plane,
> >  */
> > intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> > plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
> > +   plane_color_ctl = intel_de_read_fw(dev_priv, PLANE_COLOR_CTL(pipe,
> > +plane_id));
> >
> > if (intel_pxp_is_active(_priv->gt.pxp) &&
> > -   plane_state->plane_decryption)
> > +   plane_state->plane_decryption) {
> > plane_surf |= PLANE_SURF_DECRYPTION_ENABLED;
> > -   else
> > +   plane_color_ctl &= ~PLANE_COLOR_PLANE_CSC_ENABLE;
> > +   } else if (plane_state->plane_decryption) {
> > +   intel_load_plane_csc_black(plane);
> > +   plane_color_ctl |= PLANE_COLOR_PLANE_CSC_ENABLE;
> > +   } else {
> > plane_surf &= ~PLANE_SURF_DECRYPTION_ENABLED;
> > +   plane_color_ctl &= ~PLANE_COLOR_PLANE_CSC_ENABLE;
> > +   }
> >
> > +   intel_de_write_fw(dev_priv, PLANE_COLOR_CTL(pipe, plane_id),
> > + plane_color_ctl);
> > intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), plane_surf);
> >
> > if (plane_state->scaler_id >= 0)
> > @@ -1066,6 +1102,7 @@ skl_plane_async_flip(struct intel_plane *plane,
> > enum pipe pipe = plane->pipe;
> > u32 surf_addr = plane_state->view.color_plane[0].offset;
> > u32 plane_ctl = plane_state->ctl;
> > +   u32 plane_color_ctl = 0;
> >
> > plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> >
> > @@ -1075,6 +1112,16 @@ skl_plane_async_flip(struct intel_plane *plane,
> > spin_lock_irqsave(_priv->uncore.lock, irqflags);
> >
> > intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> > +
> > +   if

Re: [Intel-gfx] [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support

2021-04-30 Thread Gupta, Anshuman




> -Original Message-
> From: Ville Syrjälä 
> Sent: Wednesday, April 28, 2021 12:25 AM
> To: Gupta, Anshuman 
> Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo ;
> Bommu, Krishnaiah ; Huang Sean Z
> ; Gaurav, Kumar ;
> Ceraolo Spurio, Daniele 
> Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support
> 
> On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote:
> > Add support to enable/disable PLANE_SURF Decryption Request bit.
> > It requires only to enable plane decryption support when following
> > condition met.
> > 1. PXP session is enabled.
> > 2. Buffer object is protected.
> >
> > v2:
> > - Used gen fb obj user_flags instead gem_object_metadata. [Krishna]
> >
> > v3:
> > - intel_pxp_gem_object_status() API changes.
> >
> > v4: use intel_pxp_is_active (Daniele)
> >
> > v5: rebase and use the new protected object status checker (Daniele)
> >
> > v6: used plane state for plane_decryption to handle async flip
> > as suggested by Ville.
> >
> > Cc: Bommu Krishnaiah 
> > Cc: Huang Sean Z 
> > Cc: Gaurav Kumar 
> > Cc: Ville Syrjälä 
> > Signed-off-by: Anshuman Gupta 
> > Signed-off-by: Daniele Ceraolo Spurio
> > 
> > ---
> >  .../gpu/drm/i915/display/intel_atomic_plane.c |  3 ++
> > drivers/gpu/drm/i915/display/intel_display.c  |  5 +++
> >  .../drm/i915/display/intel_display_types.h|  3 ++
> >  .../drm/i915/display/skl_universal_plane.c| 32 +--
> >  .../drm/i915/display/skl_universal_plane.h|  1 +
> >  drivers/gpu/drm/i915/i915_reg.h   |  1 +
> >  6 files changed, 42 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 7bfb26ca0bd0..7057077a2b71 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct
> intel_atomic_state *state,
> > intel_atomic_get_old_crtc_state(state, crtc);
> > struct intel_crtc_state *new_crtc_state =
> > intel_atomic_get_new_crtc_state(state, crtc);
> > +   const struct drm_framebuffer *fb = new_plane_state->hw.fb;
> >
> > if (new_crtc_state && new_crtc_state->bigjoiner_slave) {
> > struct intel_plane *master_plane =
> > @@ -409,6 +410,8 @@ int intel_plane_atomic_check(struct
> intel_atomic_state *state,
> > intel_plane_copy_uapi_to_hw_state(new_plane_state,
> >   new_master_plane_state,
> >   crtc);
> > +   new_plane_state->plane_decryption =
> > +   i915_gem_object_has_valid_protection(intel_fb_obj(fb));
> >
> > new_plane_state->uapi.visible = false;
> > if (!new_crtc_state)
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index a10e26380ef3..55ab2d0b92d8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct
> intel_atomic_state *state)
> > drm_dbg_kms(>drm, "Color range cannot be
> changed in async flip\n");
> > return -EINVAL;
> > }
> > +
> > +   /* plane decryption is allow to change only in synchronous flips
> */
> > +   if (old_plane_state->plane_decryption != new_plane_state-
> >plane_decryption)
> > +   return -EINVAL;
> > }
> >
> > return 0;
> > @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct
> > drm_i915_private *dev_priv)
> >
> > crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > crtc_state = to_intel_crtc_state(crtc->base.state);
> > +   intel_plane_read_hw_decryption(plane_state);
> 
> We don't have real plane state readout anyway, so seems pointless.
could u please guide me the correct place to read the old plane decrypt
State, some where we do need to populate the plane decrypt state with plane
surface register decrypt bit.
How about plane->get_hw_state ?
Thanks,
Anshuman Gupta. 
> 
> >
> > intel_set_plane_visible(crtc_state, plane_state, visible);
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index e2e707c4dff5..76b3bb64a36a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -617,6 +617,9 @@ struct intel_plane_state {
> >
> > struct intel_fb_view view;
> >
> > +   /* Plane pxp decryption state */
> > +   bool plane_decryption;
> > +
> 
> It's all about the plane, so the plane_ prefix is entirely redundant.
> Could just call it "decrypt" I guess.
> 
> > /* plane control register */
> > u32 ctl;
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >

Re: [Intel-gfx] [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support

2021-04-30 Thread Gupta, Anshuman



> -Original Message-
> From: Ceraolo Spurio, Daniele 
> Sent: Thursday, April 29, 2021 2:10 AM
> To: Ville Syrjälä 
> Cc: Gupta, Anshuman ; Vivi, Rodrigo
> ; intel-gfx@lists.freedesktop.org; Bommu, Krishnaiah
> ; Gaurav, Kumar 
> Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption support
> 
> 
> 
> On 4/28/2021 1:04 PM, Ville Syrjälä wrote:
> > On Wed, Apr 28, 2021 at 10:32:46AM -0700, Daniele Ceraolo Spurio wrote:
> >>
> >> On 4/28/2021 5:03 AM, Ville Syrjälä wrote:
> >>> On Wed, Apr 28, 2021 at 11:25:23AM +, Gupta, Anshuman wrote:
> > -Original Message-
> > From: Ville Syrjälä 
> > Sent: Wednesday, April 28, 2021 12:25 AM
> > To: Gupta, Anshuman 
> > Cc: intel-gfx@lists.freedesktop.org; Vivi, Rodrigo
> > ; Bommu, Krishnaiah
> > ; Huang Sean Z
> > ; Gaurav, Kumar ;
> > Ceraolo Spurio, Daniele 
> > Subject: Re: [PATCH v3 14/16] drm/i915/pxp: Add plane decryption
> > support
>  Thanks Ville for review, Below are some doubts with respect to pxp state.
> > On Tue, Apr 27, 2021 at 04:13:11PM +0530, Anshuman Gupta wrote:
> >> Add support to enable/disable PLANE_SURF Decryption Request bit.
> >> It requires only to enable plane decryption support when
> >> following condition met.
> >> 1. PXP session is enabled.
> >> 2. Buffer object is protected.
> >>
> >> v2:
> >> - Used gen fb obj user_flags instead gem_object_metadata.
> >> [Krishna]
> >>
> >> v3:
> >> - intel_pxp_gem_object_status() API changes.
> >>
> >> v4: use intel_pxp_is_active (Daniele)
> >>
> >> v5: rebase and use the new protected object status checker
> >> (Daniele)
> >>
> >> v6: used plane state for plane_decryption to handle async flip
> >>   as suggested by Ville.
> >>
> >> Cc: Bommu Krishnaiah 
> >> Cc: Huang Sean Z 
> >> Cc: Gaurav Kumar 
> >> Cc: Ville Syrjälä 
> >> Signed-off-by: Anshuman Gupta 
> >> Signed-off-by: Daniele Ceraolo Spurio
> >> 
> >> ---
> >>.../gpu/drm/i915/display/intel_atomic_plane.c |  3 ++
> >> drivers/gpu/drm/i915/display/intel_display.c  |  5 +++
> >>.../drm/i915/display/intel_display_types.h|  3 ++
> >>.../drm/i915/display/skl_universal_plane.c| 32 
> >> +-
> -
> >>.../drm/i915/display/skl_universal_plane.h|  1 +
> >>drivers/gpu/drm/i915/i915_reg.h   |  1 +
> >>6 files changed, 42 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> >> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> >> index 7bfb26ca0bd0..7057077a2b71 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> >> @@ -394,6 +394,7 @@ int intel_plane_atomic_check(struct
> > intel_atomic_state *state,
> >>intel_atomic_get_old_crtc_state(state, crtc);
> >>struct intel_crtc_state *new_crtc_state =
> >>intel_atomic_get_new_crtc_state(state, crtc);
> >> +  const struct drm_framebuffer *fb = new_plane_state->hw.fb;
> >>
> >>if (new_crtc_state && new_crtc_state->bigjoiner_slave) {
> >>struct intel_plane *master_plane = @@ -409,6 +410,8
> @@ int
> >> intel_plane_atomic_check(struct
> > intel_atomic_state *state,
> >>intel_plane_copy_uapi_to_hw_state(new_plane_state,
> >>  new_master_plane_state,
> >>  crtc);
> >> +  new_plane_state->plane_decryption =
> >> +
>   i915_gem_object_has_valid_protection(intel_fb_obj(fb));
> >>
> >>new_plane_state->uapi.visible = false;
> >>if (!new_crtc_state)
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >> b/drivers/gpu/drm/i915/display/intel_display.c
> >> index a10e26380ef3..55ab2d0b92d8 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -9367,6 +9367,10 @@ static int intel_atomic_check_async(struct
> > intel_atomic_state *state)
> >>drm_dbg_kms(>drm, "Color range
> cannot be
> > changed in async flip\n");
> >>return -EINVAL;
> >>}
> >> +
> >> +  /* plane decryption is allow to change only in
> synchronous
> >> +flips
> > */
> >> +  if (old_plane_state->plane_decryption !=
> new_plane_state-
> >> plane_decryption)
> >> +  return -EINVAL;
> >>}
> >>
> >>return 0;
> >> @@ -12350,6 +12354,7 @@ static void readout_plane_state(struct
> >> drm_i915_private *dev_priv)
> >>
> >>crtc =

Re: [Intel-gfx] [PATCH 16/21] drm/i915/gem: Delay context creation