Re: [PATCH 14/16] drm/i915/vm_bind: Handle persistent vmas in execbuf3
On Mon, Oct 03, 2022 at 09:36:38AM +0100, Matthew Auld wrote:
On 02/10/2022 07:28, Niranjana Vishwanathapura wrote:
On Fri, Sep 30, 2022 at 10:47:48AM +0100, Matthew Auld wrote:
On 28/09/2022 07:19, Niranjana Vishwanathapura wrote:
Handle persistent (VM_BIND) mappings during the request submission
in the execbuf3 path.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 188 +-
1 file changed, 187 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
index 92af88bc8deb..1aeeff5e8540 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -19,6 +19,7 @@
#include "i915_gem_vm_bind.h"
#include "i915_trace.h"
+#define __EXEC3_HAS_PIN BIT_ULL(33)
#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
#define __EXEC3_INTERNAL_FLAGS (~0ull << 32)
@@ -42,7 +43,9 @@
* execlist. Hence, no support for implicit sync.
*
* The new execbuf3 ioctl only works in VM_BIND mode and the
VM_BIND mode only
- * works with execbuf3 ioctl for submission.
+ * works with execbuf3 ioctl for submission. All BOs mapped on
that VM (through
+ * VM_BIND call) at the time of execbuf3 call are deemed
required for that
+ * submission.
*
* The execbuf3 ioctl directly specifies the batch addresses
instead of as
* object handles as in execbuf2 ioctl. The execbuf3 ioctl will
also not
@@ -58,6 +61,13 @@
* So, a lot of code supporting execbuf2 ioctl, like
relocations, VA evictions,
* vma lookup table, implicit sync, vma active reference
tracking etc., are not
* applicable for execbuf3 ioctl.
+ *
+ * During each execbuf submission, request fence is added to
all VM_BIND mapped
+ * objects with DMA_RESV_USAGE_BOOKKEEP. The
DMA_RESV_USAGE_BOOKKEEP usage will
+ * prevent over sync (See enum dma_resv_usage). Note that
DRM_I915_GEM_WAIT and
+ * DRM_I915_GEM_BUSY ioctls do not check for
DMA_RESV_USAGE_BOOKKEEP usage and
+ * hence should not be used for end of batch check. Instead,
the execbuf3
+ * timeline out fence should be used for end of batch check.
*/
/**
@@ -127,6 +137,23 @@ eb_find_vma(struct i915_address_space *vm,
u64 addr)
return i915_gem_vm_bind_lookup_vma(vm, va);
}
+static void eb_scoop_unbound_vma_all(struct i915_address_space *vm)
+{
+ struct i915_vma *vma, *vn;
+
+ /**
+ * Move all unbound vmas back into vm_bind_list so that they are
+ * revalidated.
+ */
+ spin_lock(>vm_rebind_lock);
+ list_for_each_entry_safe(vma, vn, >vm_rebind_list,
vm_rebind_link) {
+ list_del_init(>vm_rebind_link);
+ if (!list_empty(>vm_bind_link))
+ list_move_tail(>vm_bind_link, >vm_bind_list);
+ }
+ spin_unlock(>vm_rebind_lock);
+}
+
static int eb_lookup_vma_all(struct i915_execbuffer *eb)
{
unsigned int i, current_batch = 0;
@@ -141,14 +168,121 @@ static int eb_lookup_vma_all(struct
i915_execbuffer *eb)
++current_batch;
}
+ eb_scoop_unbound_vma_all(eb->context->vm);
+
+ return 0;
+}
+
+static int eb_lock_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int err;
+
+ err = i915_gem_object_lock(eb->context->vm->root_obj, >ww);
+ if (err)
+ return err;
+
+ list_for_each_entry(vma, >non_priv_vm_bind_list,
+ non_priv_vm_bind_link) {
+ err = i915_gem_object_lock(vma->obj, >ww);
+ if (err)
+ return err;
+ }
+
return 0;
}
+static void eb_release_persistent_vma_all(struct i915_execbuffer *eb,
+ bool final)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma, *vn;
+
+ lockdep_assert_held(>vm_bind_lock);
+
+ if (!(eb->args->flags & __EXEC3_HAS_PIN))
+ return;
+
+ assert_object_held(vm->root_obj);
+
+ list_for_each_entry(vma, >vm_bind_list, vm_bind_link)
+ __i915_vma_unpin(vma);
+
+ eb->args->flags &= ~__EXEC3_HAS_PIN;
+ if (!final)
+ return;
+
+ list_for_each_entry_safe(vma, vn, >vm_bind_list, vm_bind_link)
+ if (i915_vma_verify_bind_complete(vma))
+ list_move_tail(>vm_bind_link, >vm_bound_list);
+}
+
static void eb_release_vma_all(struct i915_execbuffer *eb, bool final)
{
+ eb_release_persistent_vma_all(eb, final);
eb_unpin_engine(eb);
}
+static int eb_reserve_fence_for_persistent_vma_all(struct
i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int ret;
+
+ ret = dma_resv_reserve_fences(vm->root_obj->base.resv, 1);
+ if (ret)
+ return ret;
+
+ list_for_each_entry(vma, >non_priv_vm_bind_list,
+ non_priv_vm_bind_link) {
+ ret = dma_resv_reserve_fences(vma->obj->base.resv, 1);
+ if (ret)
+ return ret;
+ }
+
+
Re: [PATCH v2 12/14] drm/i915: Define multicast registers as a new type
On Tue, Oct 04, 2022 at 04:00:57PM +0300, Jani Nikula wrote:
> On Tue, 04 Oct 2022, Jani Nikula wrote:
> > On Fri, 30 Sep 2022, Matt Roper wrote:
> >> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h
> >> b/drivers/gpu/drm/i915/i915_reg_defs.h
> >> index 8f486f77609f..e823869b9afd 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
> >> @@ -104,22 +104,16 @@ typedef struct {
> >>
> >> #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
> >>
> >> -#define INVALID_MMIO_REG _MMIO(0)
> >> -
> >> -static __always_inline u32 i915_mmio_reg_offset(i915_reg_t reg)
> >> -{
> >> - return reg.reg;
> >> -}
> >> +typedef struct {
> >> + u32 reg;
> >> +} i915_mcr_reg_t;
> >>
> >> -static inline bool i915_mmio_reg_equal(i915_reg_t a, i915_reg_t b)
> >> -{
> >> - return i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b);
> >> -}
> >> +#define INVALID_MMIO_REG _MMIO(0)
> >>
> >> -static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >> -{
> >> - return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
> >> -}
> >> +/* These macros can be used on either i915_reg_t or i915_mcr_reg_t */
> >> +#define i915_mmio_reg_offset(r) (r.reg)
> >> +#define i915_mmio_reg_equal(a, b) (i915_mmio_reg_offset(a) ==
> >> i915_mmio_reg_offset(b))
> >> +#define i915_mmio_reg_valid(r) (!i915_mmio_reg_equal(r, INVALID_MMIO_REG))
> >>
> >
> > I don't really like losing the type safety here. The whole and only
> > purpose of typedeffing i915_reg_t as a struct instead of just u32 was
> > the strict type safety.
>
> PS. Changes like this should really be highlighted better, in the commit
> subject and title. Now it feels like it's hidden within a big commit
> within a big series, without even mentioning it in the commit message.
This patch adds the type safety we've been missing until now. The only
thing that's slightly less safe is the i915_mmio_reg_offset() macro
itself since it allows either of the two split register types to be
passed to the same macro for simplicity. In theory if you had some
other structure that also had a 'reg' member it could sneak through
here, but most type mistakes (e.g., passing an integer) would still
cause a build failure.
If we want to make this specific macro more bullet-proof, while still
minimizing thrash elsewhere, we could re-write this as
#define i915_mmio_reg_offset(r) \
_Generic((r), i915_reg_t: (r).reg, i915_mcr_reg_t: (r).reg)
which I believe should result in a build failure if anything other than
an i915_reg_t or i915_mcr_reg_t is passed in.
We could also just split this out into separate MCR vs non-MCR offset
lookup functions as we've done with other things in this series. But if
I recall correctly that leads to annoying thrash in stuff like
cmdparser, gvt, perf, etc. that never actually wanted registers to begin
with, but rather lists/ranges of MMIO offsets without a care for the
type of register that lives at the offset.
Matt
>
>
> BR,
> Jani.
>
>
> >
> > BR,
> > Jani.
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
linux-next: manual merge of the drm-misc tree with Linus' tree
Hi all,
Today's linux-next merge of the drm-misc tree got a conflict in:
include/drm/drm_edid.h
between commit:
c7943bb324e5 ("drm/edid: Handle EDID 1.4 range descriptor h/vfreq offsets")
from Linus' tree and commit:
afd4429eba28 ("drm/edid: Define more flags")
from the drm-misc tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc include/drm/drm_edid.h
index 1ed61e2b30a4,28dd80343afa..
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@@ -92,15 -92,13 +92,18 @@@ struct detailed_data_string
u8 str[13];
} __attribute__((packed));
+#define DRM_EDID_RANGE_OFFSET_MIN_VFREQ (1 << 0) /* 1.4 */
+#define DRM_EDID_RANGE_OFFSET_MAX_VFREQ (1 << 1) /* 1.4 */
+#define DRM_EDID_RANGE_OFFSET_MIN_HFREQ (1 << 2) /* 1.4 */
+#define DRM_EDID_RANGE_OFFSET_MAX_HFREQ (1 << 3) /* 1.4 */
+
- #define DRM_EDID_DEFAULT_GTF_SUPPORT_FLAG 0x00
- #define DRM_EDID_RANGE_LIMITS_ONLY_FLAG 0x01
- #define DRM_EDID_SECONDARY_GTF_SUPPORT_FLAG 0x02
- #define DRM_EDID_CVT_SUPPORT_FLAG 0x04
+ #define DRM_EDID_DEFAULT_GTF_SUPPORT_FLAG 0x00 /* 1.3 */
+ #define DRM_EDID_RANGE_LIMITS_ONLY_FLAG 0x01 /* 1.4 */
+ #define DRM_EDID_SECONDARY_GTF_SUPPORT_FLAG 0x02 /* 1.3 */
+ #define DRM_EDID_CVT_SUPPORT_FLAG 0x04 /* 1.4 */
+
+ #define DRM_EDID_CVT_FLAGS_STANDARD_BLANKING (1 << 3)
+ #define DRM_EDID_CVT_FLAGS_REDUCED_BLANKING (1 << 4)
struct detailed_data_monitor_range {
u8 min_vfreq;
pgpEgCImF1NTK.pgp
Description: OpenPGP digital signature
[PATCH] drm/i915/mtl: Add MTP ddc pin configuration
Meteorlake PCH reuses Alderlake ddc pin mapping. Extend
ADL-P pin mapping for Meteorlake.
Cc: Lucas De Marchi
Signed-off-by: Radhakrishna Sripada
---
drivers/gpu/drm/i915/display/intel_bios.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
b/drivers/gpu/drm/i915/display/intel_bios.c
index 4c543e8205ca..c2987f2c2b2e 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -2188,7 +2188,7 @@ static u8 map_ddc_pin(struct drm_i915_private *i915, u8
vbt_pin)
const u8 *ddc_pin_map;
int n_entries;
- if (IS_ALDERLAKE_P(i915)) {
+ if (HAS_PCH_MTP(i915) || IS_ALDERLAKE_P(i915)) {
ddc_pin_map = adlp_ddc_pin_map;
n_entries = ARRAY_SIZE(adlp_ddc_pin_map);
} else if (IS_ALDERLAKE_S(i915)) {
--
2.34.1
Re: [PATCH v3 2/2] drm/msm/dsi: Add phy configuration for QCM2290
On 2022-10-04 17:29:32, Dmitry Baryshkov wrote: > On Sat, 1 Oct 2022 at 19:00, Marijn Suijten > wrote: > > [..] > > For sm6125 we also need this exact io_start (and a single PHY), do you > > think it makes sense to add a compatible that reuses the same struct (I > > can do that in a folloup patch) and/or generalize this struct (name)? > > > > However, our regulator setup appears to be different. I recall not > > finding any `vcca` supply in my downstream sources, and had this in my > > notes for a similar dsi_phy_14nm.c patch: > > > > sm6125 uses an RPM regulator > > > > https://github.com/sonyxperiadev/kernel/blob/f956fbd9a234033bd18234d456a2c32c126b38f3/arch/arm64/boot/dts/qcom/trinket-sde.dtsi#L388 > > I'd prefer a separate config for sm6125. This way you would be able to > add voting on the MX domain if required. Ack, I'll queue up a patch series for this SoC, with a dt-bindings patch that makes the vcca register optional for the sm6125 compatible. - Marijn
Re: [PATCH 16/16] drm/udl: Add constants for commands
On 9/19/22 15:04, Thomas Zimmermann wrote: > Add constants for the various commands that the driver can send to > the device and update the respective helper functions. No functional > changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 15/16] drm/udl: Add register constants for framebuffer scanout addresses
On 9/19/22 15:04, Thomas Zimmermann wrote: > Add register constants for the framebuffer scanout addresses and > update the related helper functions. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas [...] > + u8 reg20 = (base & 0xff) >> 16; > + u8 reg21 = (base & 0x00ff00) >> 8; > + u8 reg22 = (base & 0xff); > + Maybe would be cleaner to use the FIELD_PREP() and GENMASK() macros instead ? -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits
On 2022-10-05 01:40:12, Dmitry Baryshkov wrote: > On 05/10/2022 01:35, Marijn Suijten wrote: > > On 2022-10-04 17:45:50, Dmitry Baryshkov wrote: > >> On Sat, 1 Oct 2022 at 22:08, Marijn Suijten > >> wrote: > >> [..] > >>> - bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * > >>> dsc->bits_per_pixel, 8); > >>> + bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8); > >> > >> > >> bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8 * > >> 16); ? > > > > Not necessarily a fan of this, it "hides" the fact that we are dealing > > with 4 fractional bits (1/16th precision, it is correct though); but > > since this is the only use of `bpp` I can change it and document this > > fact wiht a comment on top (including referencing the validation pointed > > out in dsi_populate_dsc_params()). > > > > Alternatively we can inline the `>> 4` here? > > No, I don't think so. If we shift by 4 bits, we'd loose the fractional > part. DIV_ROUND_UP( , 8 * 16) ensures that we round it up rather > than just dropping it. I'd still keep the `-EINVAL` on `if (dsc->bits_per_pixel & 0xf)` to guarantee that there is no fractional part. After all, as explained in the patch description, none of this code / the DSI driver in general seems to be able to handle fractional bits per pixel. > >>> [..] > >>> - dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / > >>> 8; > >>> - if ((dsc->slice_width * dsc->bits_per_pixel) % 8) > >>> + dsc->slice_chunk_size = dsc->slice_width * bpp / 8; > >>> + if ((dsc->slice_width * bpp) % 8) > >> > >> One can use fixed point math here too: > >> > >> dsc->slice_chunk_size = (dsc->slice_width * dsc->bits_per_pixel + 8 * > >> 16 - 1)/ (8 * 16); > > > > Good catch, this is effectively a DIV_ROUND_UP() that we happened to > > call bytes_in_slice above... > > > > Shall I tackle this in the same patch, or insert another cleanup patch? > > It's up to you. I usually prefer separate patches, even if just to ease > bisecting between unrelated changes. Same feeling here, and have already set it up that way; added two extra patches to 1. replace this with DIV_ROUND_UP() and 2. remove the recalculation of slice_chunk_size (disguised as bytes_in_slice) above. - Marijn
Re: [PATCH 14/16] drm/udl: Add register constants for video locks
On 9/19/22 15:04, Thomas Zimmermann wrote: > Add register constants for the video lock. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 13/16] drm/udl: Add register constants for color depth
On 9/19/22 15:04, Thomas Zimmermann wrote: > Add the register constants for setting the color depth. The driver > only uses 16bpp. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 12/16] drm/udl: Add constants for display-mode registers
On 9/19/22 15:04, Thomas Zimmermann wrote: > Add constants for the registers the contain various display-mode > parameters and update the mode-setting function. No functional > changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits
On 05/10/2022 01:35, Marijn Suijten wrote:
On 2022-10-04 17:45:50, Dmitry Baryshkov wrote:
On Sat, 1 Oct 2022 at 22:08, Marijn Suijten
wrote:
[..]
- bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel,
8);
+ bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8);
bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8 * 16); ?
Not necessarily a fan of this, it "hides" the fact that we are dealing
with 4 fractional bits (1/16th precision, it is correct though); but
since this is the only use of `bpp` I can change it and document this
fact wiht a comment on top (including referencing the validation pointed
out in dsi_populate_dsc_params()).
Alternatively we can inline the `>> 4` here?
No, I don't think so. If we shift by 4 bits, we'd loose the fractional
part. DIV_ROUND_UP( , 8 * 16) ensures that we round it up rather
than just dropping it.
dsc->slice_chunk_size = bytes_in_slice;
@@ -913,6 +918,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host,
bool is_bonded_dsi)
u32 va_end = va_start + mode->vdisplay;
u32 hdisplay = mode->hdisplay;
u32 wc;
+ int ret;
DBG("");
@@ -948,7 +954,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host,
bool is_bonded_dsi)
/* we do the calculations for dsc parameters here so that
* panel can use these parameters
*/
- dsi_populate_dsc_params(dsc);
+ ret = dsi_populate_dsc_params(dsc);
+ if (ret)
+ return;
/* Divide the display by 3 but keep back/font porch and
* pulse width same
@@ -1229,6 +1237,10 @@ static int dsi_cmd_dma_add(struct msm_dsi_host *msm_host,
if (packet.size < len)
memset(data + packet.size, 0xff, len - packet.size);
+ if (msg->type == MIPI_DSI_PICTURE_PARAMETER_SET)
+ print_hex_dump(KERN_DEBUG, "ALL:", DUMP_PREFIX_NONE,
+ 16, 1, data, len, false);
+
if (cfg_hnd->ops->tx_buf_put)
cfg_hnd->ops->tx_buf_put(msm_host);
@@ -1786,6 +1798,12 @@ static int dsi_populate_dsc_params(struct drm_dsc_config
*dsc)
int data;
int final_value, final_scale;
int i;
+ int bpp = dsc->bits_per_pixel >> 4;
+
+ if (dsc->bits_per_pixel & 0xf) {
+ pr_err("DSI does not support fractional bits_per_pixel\n");
+ return -EINVAL;
+ }
dsc->rc_model_size = 8192;
dsc->first_line_bpg_offset = 12;
@@ -1807,7 +1825,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config
*dsc)
}
dsc->initial_offset = 6144; /* Not bpp 12 */
- if (dsc->bits_per_pixel != 8)
+ if (bpp != 8)
dsc->initial_offset = 2048; /* bpp = 12 */
mux_words_size = 48;/* bpc == 8/10 */
@@ -1830,16 +1848,16 @@ static int dsi_populate_dsc_params(struct
drm_dsc_config *dsc)
* params are calculated
*/
groups_per_line = DIV_ROUND_UP(dsc->slice_width, 3);
- dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8;
- if ((dsc->slice_width * dsc->bits_per_pixel) % 8)
+ dsc->slice_chunk_size = dsc->slice_width * bpp / 8;
+ if ((dsc->slice_width * bpp) % 8)
One can use fixed point math here too:
dsc->slice_chunk_size = (dsc->slice_width * dsc->bits_per_pixel + 8 *
16 - 1)/ (8 * 16);
Good catch, this is effectively a DIV_ROUND_UP() that we happened to
call bytes_in_slice above...
Shall I tackle this in the same patch, or insert another cleanup patch?
It's up to you. I usually prefer separate patches, even if just to ease
bisecting between unrelated changes.
In fact dsi_populate_dsc_params() is called first (this comment),
followed by dsi_update_dsc_timing(), meaning that slice_chunk_size is
already provided and shouldn't be recomputed.
dsc->slice_chunk_size++;
/* rbs-min */
min_rate_buffer_size = dsc->rc_model_size - dsc->initial_offset +
- dsc->initial_xmit_delay * dsc->bits_per_pixel +
+ dsc->initial_xmit_delay * bpp +
groups_per_line * dsc->first_line_bpg_offset;
- hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, dsc->bits_per_pixel);
+ hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, bpp);
dsc->initial_dec_delay = hrd_delay - dsc->initial_xmit_delay;
@@ -1862,7 +1880,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config
*dsc)
data = 2048 * (dsc->rc_model_size - dsc->initial_offset +
num_extra_mux_bits);
dsc->slice_bpg_offset = DIV_ROUND_UP(data, groups_total);
- target_bpp_x16 = dsc->bits_per_pixel * 16;
+ target_bpp_x16 = bpp * 16;
data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
It looks
Re: [PATCH 11/16] drm/udl: Move register constants to udl_proto.h
On 9/19/22 15:04, Thomas Zimmermann wrote: > Move the existing register constants to a new file in preparation of > adding more of them. Renaming is intentional. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields
On 2022-10-04 15:31:10, Abhinav Kumar wrote: > > > On 10/4/2022 2:57 PM, Marijn Suijten wrote: > > [..] > > Alas, as explained in the cover letter I opted to perform the masking in > > the PPS packing code as the DSC block code also reads these values, and > > would suddenly write 6-bit intead of 8-bit values to the > > DSC_RANGE_BPG_OFFSET registers. Quick testing on the mentioned sdm845 > > platform shows no regressions, but I'm not sure if that's safe to rely > > on? > > I looked up the MDP_DSC_0_RANGE_BPG_OFFSET_* registers. > They take only a 6-bit value according to the SW documentation ( bits 5:0 ) > > It was always expecting only a 6-bit value and not 8. > > So this change is safe. Ack, I think that implies I should make this change and move the masks to the DSI driver? > >> If you want to move to helper, other drivers need to be changed too to > >> remove duplicate & 0x3f. > > > > Sure, we only have to confirm whether those drivers also read back the > > value(s) in rc_range_params, and expect / allow this to be 8 instead of > > 6 bits. > > > >> FWIW, this too has already been fixed in the latest downstream driver too. > > > > What is this supposed to mean? Is there a downstream DPU project that > > has pending patches needing to be upstreamed? Or is the downstream SDE, > > techpack/display, or whatever it is called nowadays, slowly using more > > DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack() > > helper function as pointed out in an earlier mail? > > > > No, what I meant was, the version of downstream driver based on which > the upstream DSC was made seems to be an older version. Downstream > drivers keep getting updated and we always keep trying to align with > upstream structs. > > This is true not just for DSC but even other blocks. > > So as part of that effort, we started using struct drm_dsc_config . That > change was made on newer chipsets. But the downstream SW on sdm845 based > on which the DSC was upstreamed seems like didnt have that. Hence all > this redundant math happened. > > So this comment was more of a explanation about why this issue happened > even though latest downstream didnt have this issue. Thanks, I understood most of that but wasn't aware these exact "issues" were also addressed downstream (by i.e. also using the upstream structs). > > Offtopic: are SDE and DPU growing closer together, hopefully achieving > > feature parity allowing the SDE project to be dropped in favour of a > > fully upstreamed DPU driver for day-one out-of-the-box mainline support > > for new SoCs (as long as work is published and on its way upstream)? > > > > There is still a lot of gap between SDE and DPU drivers at this point. > We keep trying to upstream as many features as possible to minimize the > gap but there is still a lot of work to do. Glad to hear, but that sounds like a very hard to close gap unless downstream "just works on DPU" instead of having parallel development on two "competing" drivers for the exact same hardware. - Marijn
Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits
On 2022-10-04 17:45:50, Dmitry Baryshkov wrote:
> On Sat, 1 Oct 2022 at 22:08, Marijn Suijten
> wrote:
> [..]
> > - bytes_in_slice = DIV_ROUND_UP(dsc->slice_width *
> > dsc->bits_per_pixel, 8);
> > + bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8);
>
>
> bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8 *
> 16); ?
Not necessarily a fan of this, it "hides" the fact that we are dealing
with 4 fractional bits (1/16th precision, it is correct though); but
since this is the only use of `bpp` I can change it and document this
fact wiht a comment on top (including referencing the validation pointed
out in dsi_populate_dsc_params()).
Alternatively we can inline the `>> 4` here?
> >
> > dsc->slice_chunk_size = bytes_in_slice;
> >
> > @@ -913,6 +918,7 @@ static void dsi_timing_setup(struct msm_dsi_host
> > *msm_host, bool is_bonded_dsi)
> > u32 va_end = va_start + mode->vdisplay;
> > u32 hdisplay = mode->hdisplay;
> > u32 wc;
> > + int ret;
> >
> > DBG("");
> >
> > @@ -948,7 +954,9 @@ static void dsi_timing_setup(struct msm_dsi_host
> > *msm_host, bool is_bonded_dsi)
> > /* we do the calculations for dsc parameters here so that
> > * panel can use these parameters
> > */
> > - dsi_populate_dsc_params(dsc);
> > + ret = dsi_populate_dsc_params(dsc);
> > + if (ret)
> > + return;
> >
> > /* Divide the display by 3 but keep back/font porch and
> > * pulse width same
> > @@ -1229,6 +1237,10 @@ static int dsi_cmd_dma_add(struct msm_dsi_host
> > *msm_host,
> > if (packet.size < len)
> > memset(data + packet.size, 0xff, len - packet.size);
> >
> > + if (msg->type == MIPI_DSI_PICTURE_PARAMETER_SET)
> > + print_hex_dump(KERN_DEBUG, "ALL:", DUMP_PREFIX_NONE,
> > + 16, 1, data, len, false);
> > +
> > if (cfg_hnd->ops->tx_buf_put)
> > cfg_hnd->ops->tx_buf_put(msm_host);
> >
> > @@ -1786,6 +1798,12 @@ static int dsi_populate_dsc_params(struct
> > drm_dsc_config *dsc)
> > int data;
> > int final_value, final_scale;
> > int i;
> > + int bpp = dsc->bits_per_pixel >> 4;
> > +
> > + if (dsc->bits_per_pixel & 0xf) {
> > + pr_err("DSI does not support fractional bits_per_pixel\n");
> > + return -EINVAL;
> > + }
> >
> > dsc->rc_model_size = 8192;
> > dsc->first_line_bpg_offset = 12;
> > @@ -1807,7 +1825,7 @@ static int dsi_populate_dsc_params(struct
> > drm_dsc_config *dsc)
> > }
> >
> > dsc->initial_offset = 6144; /* Not bpp 12 */
> > - if (dsc->bits_per_pixel != 8)
> > + if (bpp != 8)
> > dsc->initial_offset = 2048; /* bpp = 12 */
> >
> > mux_words_size = 48;/* bpc == 8/10 */
> > @@ -1830,16 +1848,16 @@ static int dsi_populate_dsc_params(struct
> > drm_dsc_config *dsc)
> > * params are calculated
> > */
> > groups_per_line = DIV_ROUND_UP(dsc->slice_width, 3);
> > - dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8;
> > - if ((dsc->slice_width * dsc->bits_per_pixel) % 8)
> > + dsc->slice_chunk_size = dsc->slice_width * bpp / 8;
> > + if ((dsc->slice_width * bpp) % 8)
>
> One can use fixed point math here too:
>
> dsc->slice_chunk_size = (dsc->slice_width * dsc->bits_per_pixel + 8 *
> 16 - 1)/ (8 * 16);
Good catch, this is effectively a DIV_ROUND_UP() that we happened to
call bytes_in_slice above...
Shall I tackle this in the same patch, or insert another cleanup patch?
In fact dsi_populate_dsc_params() is called first (this comment),
followed by dsi_update_dsc_timing(), meaning that slice_chunk_size is
already provided and shouldn't be recomputed.
> > dsc->slice_chunk_size++;
> >
> > /* rbs-min */
> > min_rate_buffer_size = dsc->rc_model_size - dsc->initial_offset +
> > - dsc->initial_xmit_delay *
> > dsc->bits_per_pixel +
> > + dsc->initial_xmit_delay * bpp +
> > groups_per_line *
> > dsc->first_line_bpg_offset;
> >
> > - hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, dsc->bits_per_pixel);
> > + hrd_delay = DIV_ROUND_UP(min_rate_buffer_size, bpp);
> >
> > dsc->initial_dec_delay = hrd_delay - dsc->initial_xmit_delay;
> >
> > @@ -1862,7 +1880,7 @@ static int dsi_populate_dsc_params(struct
> > drm_dsc_config *dsc)
> > data = 2048 * (dsc->rc_model_size - dsc->initial_offset +
> > num_extra_mux_bits);
> > dsc->slice_bpg_offset = DIV_ROUND_UP(data, groups_total);
> >
> > - target_bpp_x16 = dsc->bits_per_pixel * 16;
> > + target_bpp_x16 = bpp * 16;
> >
> > data = (dsc->initial_xmit_delay *
Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields
On 10/4/2022 2:57 PM, Marijn Suijten wrote:
On 2022-10-04 13:22:25, Abhinav Kumar wrote:
On 10/1/2022 12:08 PM, Marijn Suijten wrote:
msm's dsi_host specifies negative BPG offsets which fill the full 8 bits
of a char thanks to two's complement: this however results in those bits
bleeding into the next parameter when the field is only expected to
contain 6-bit wide values.
As a consequence random slices appear corrupted on-screen (tested on a
Sony Tama Akatsuki device with sdm845).
Use AND operators to limit all values that constitute the RC Range
parameter fields to their expected size.
Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c
b/drivers/gpu/drm/display/drm_dsc_helper.c
index c869c6e51e2b..2e7ef242685d 100644
--- a/drivers/gpu/drm/display/drm_dsc_helper.c
+++ b/drivers/gpu/drm/display/drm_dsc_helper.c
@@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct
drm_dsc_picture_parameter_set *pps_payload,
*/
for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
pps_payload->rc_range_parameters[i] =
- cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
+ cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp &
0x1f) <<
DSC_PPS_RC_RANGE_MINQP_SHIFT) |
- (dsc_cfg->rc_range_params[i].range_max_qp <<
+ ((dsc_cfg->rc_range_params[i].range_max_qp &
0x1f) <<
DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
-
(dsc_cfg->rc_range_params[i].range_bpg_offset));
+ (dsc_cfg->rc_range_params[i].range_bpg_offset
& 0x3f));
}
Looking at some examples of this for other vendors, they have managed to
limit the value to 6 bits in their drivers:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L532
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dsc/rc_calc_dpi.c#L87
Perhaps, msm should do the same thing instead of the helper change.
Thanks, I should have done my due-diligence and look up how other
drivers dealt with this, but wasn't immediately expecting negative
values elsewhere.
Alas, as explained in the cover letter I opted to perform the masking in
the PPS packing code as the DSC block code also reads these values, and
would suddenly write 6-bit intead of 8-bit values to the
DSC_RANGE_BPG_OFFSET registers. Quick testing on the mentioned sdm845
platform shows no regressions, but I'm not sure if that's safe to rely
on?
I looked up the MDP_DSC_0_RANGE_BPG_OFFSET_* registers.
They take only a 6-bit value according to the SW documentation ( bits 5:0 )
It was always expecting only a 6-bit value and not 8.
So this change is safe.
If you want to move to helper, other drivers need to be changed too to
remove duplicate & 0x3f.
Sure, we only have to confirm whether those drivers also read back the
value(s) in rc_range_params, and expect / allow this to be 8 instead of
6 bits.
FWIW, this too has already been fixed in the latest downstream driver too.
What is this supposed to mean? Is there a downstream DPU project that
has pending patches needing to be upstreamed? Or is the downstream SDE,
techpack/display, or whatever it is called nowadays, slowly using more
DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack()
helper function as pointed out in an earlier mail?
No, what I meant was, the version of downstream driver based on which
the upstream DSC was made seems to be an older version. Downstream
drivers keep getting updated and we always keep trying to align with
upstream structs.
This is true not just for DSC but even other blocks.
So as part of that effort, we started using struct drm_dsc_config . That
change was made on newer chipsets. But the downstream SW on sdm845 based
on which the DSC was upstreamed seems like didnt have that. Hence all
this redundant math happened.
So this comment was more of a explanation about why this issue happened
even though latest downstream didnt have this issue.
Offtopic: are SDE and DPU growing closer together, hopefully achieving
feature parity allowing the SDE project to be dropped in favour of a
fully upstreamed DPU driver for day-one out-of-the-box mainline support
for new SoCs (as long as work is published and on its way upstream)?
There is still a lot of gap between SDE and DPU drivers at this point.
We keep trying to upstream as many features as possible to minimize the
gap but there is still a lot of work to do.
- Marijn
[PATCH v2 2/2] drm/i915/slpc: Update the frequency debugfs
Read the values stored in the SLPC structures. Remove the
fields that are no longer valid (like RPS interrupts) as
well.
v2: Move all functionality changes to this patch (Jani)
Signed-off-by: Vinay Belgaumkar
---
drivers/gpu/drm/i915/gt/intel_rps.c | 46 -
1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
b/drivers/gpu/drm/i915/gt/intel_rps.c
index 737db780db00..8181d85e89f8 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -2219,7 +2219,7 @@ u32 intel_rps_get_rpn_frequency(struct intel_rps *rps)
return intel_gpu_freq(rps, rps->min_freq);
}
-void gen6_rps_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
+void rps_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
{
struct intel_gt *gt = rps_to_gt(rps);
struct drm_i915_private *i915 = gt->i915;
@@ -2382,6 +2382,50 @@ void gen6_rps_frequency_dump(struct intel_rps *rps,
struct drm_printer *p)
intel_gpu_freq(rps, rps->efficient_freq));
}
+static void slpc_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
+{
+ struct intel_gt *gt = rps_to_gt(rps);
+ struct intel_uncore *uncore = gt->uncore;
+ struct intel_rps_freq_caps caps;
+ u32 pm_mask;
+
+ gen6_rps_get_freq_caps(rps, );
+ pm_mask = intel_uncore_read(uncore, GEN6_PMINTRMSK);
+
+ drm_printf(p, "PM MASK=0x%08x\n", pm_mask);
+ drm_printf(p, "pm_intrmsk_mbz: 0x%08x\n",
+ rps->pm_intrmsk_mbz);
+ drm_printf(p, "RPSTAT1: 0x%08x\n", intel_uncore_read(uncore,
GEN6_RPSTAT1));
+ drm_printf(p, "RPNSWREQ: %dMHz\n",
intel_rps_get_requested_frequency(rps));
+ drm_printf(p, "Lowest (RPN) frequency: %dMHz\n",
+ intel_gpu_freq(rps, caps.min_freq));
+ drm_printf(p, "Nominal (RP1) frequency: %dMHz\n",
+ intel_gpu_freq(rps, caps.rp1_freq));
+ drm_printf(p, "Max non-overclocked (RP0) frequency: %dMHz\n",
+ intel_gpu_freq(rps, caps.rp0_freq));
+ drm_printf(p, "Current freq: %d MHz\n",
+ intel_rps_get_requested_frequency(rps));
+ drm_printf(p, "Actual freq: %d MHz\n",
+ intel_rps_read_actual_frequency(rps));
+ drm_printf(p, "Min freq: %d MHz\n",
+ intel_rps_get_min_frequency(rps));
+ drm_printf(p, "Boost freq: %d MHz\n",
+ intel_rps_get_boost_frequency(rps));
+ drm_printf(p, "Max freq: %d MHz\n",
+ intel_rps_get_max_frequency(rps));
+ drm_printf(p,
+ "efficient (RPe) frequency: %d MHz\n",
+ intel_gpu_freq(rps, caps.rp1_freq));
+}
+
+void gen6_rps_frequency_dump(struct intel_rps *rps, struct drm_printer *p)
+{
+ if (!rps_uses_slpc(rps))
+ return rps_frequency_dump(rps, p);
+ else
+ return slpc_frequency_dump(rps, p);
+}
+
static int set_max_freq(struct intel_rps *rps, u32 val)
{
struct drm_i915_private *i915 = rps_to_i915(rps);
--
2.35.1
[PATCH v2 1/2] drm/i915: Add a wrapper for frequency debugfs
Move it to the RPS source file.
v2: Separate out code movement and functional changes (Jani)
Signed-off-by: Vinay Belgaumkar
---
drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 157 +
drivers/gpu/drm/i915/gt/intel_rps.c | 163 ++
drivers/gpu/drm/i915/gt/intel_rps.h | 3 +
3 files changed, 167 insertions(+), 156 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
index 10f680dbd7b6..40d0a3be42ac 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
@@ -344,162 +344,7 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
struct drm_printer *p)
drm_printf(p, "efficient (RPe) frequency: %d MHz\n",
intel_gpu_freq(rps, rps->efficient_freq));
} else if (GRAPHICS_VER(i915) >= 6) {
- u32 rp_state_limits;
- u32 gt_perf_status;
- struct intel_rps_freq_caps caps;
- u32 rpmodectl, rpinclimit, rpdeclimit;
- u32 rpstat, cagf, reqf;
- u32 rpcurupei, rpcurup, rpprevup;
- u32 rpcurdownei, rpcurdown, rpprevdown;
- u32 rpupei, rpupt, rpdownei, rpdownt;
- u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
-
- rp_state_limits = intel_uncore_read(uncore,
GEN6_RP_STATE_LIMITS);
- gen6_rps_get_freq_caps(rps, );
- if (IS_GEN9_LP(i915))
- gt_perf_status = intel_uncore_read(uncore,
BXT_GT_PERF_STATUS);
- else
- gt_perf_status = intel_uncore_read(uncore,
GEN6_GT_PERF_STATUS);
-
- /* RPSTAT1 is in the GT power well */
- intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
-
- reqf = intel_uncore_read(uncore, GEN6_RPNSWREQ);
- if (GRAPHICS_VER(i915) >= 9) {
- reqf >>= 23;
- } else {
- reqf &= ~GEN6_TURBO_DISABLE;
- if (IS_HASWELL(i915) || IS_BROADWELL(i915))
- reqf >>= 24;
- else
- reqf >>= 25;
- }
- reqf = intel_gpu_freq(rps, reqf);
-
- rpmodectl = intel_uncore_read(uncore, GEN6_RP_CONTROL);
- rpinclimit = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
- rpdeclimit = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
-
- rpstat = intel_uncore_read(uncore, GEN6_RPSTAT1);
- rpcurupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) &
GEN6_CURICONT_MASK;
- rpcurup = intel_uncore_read(uncore, GEN6_RP_CUR_UP) &
GEN6_CURBSYTAVG_MASK;
- rpprevup = intel_uncore_read(uncore, GEN6_RP_PREV_UP) &
GEN6_CURBSYTAVG_MASK;
- rpcurdownei = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN_EI) &
GEN6_CURIAVG_MASK;
- rpcurdown = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN) &
GEN6_CURBSYTAVG_MASK;
- rpprevdown = intel_uncore_read(uncore, GEN6_RP_PREV_DOWN) &
GEN6_CURBSYTAVG_MASK;
-
- rpupei = intel_uncore_read(uncore, GEN6_RP_UP_EI);
- rpupt = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
-
- rpdownei = intel_uncore_read(uncore, GEN6_RP_DOWN_EI);
- rpdownt = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
-
- cagf = intel_rps_read_actual_frequency(rps);
-
- intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
-
- if (GRAPHICS_VER(i915) >= 11) {
- pm_ier = intel_uncore_read(uncore,
GEN11_GPM_WGBOXPERF_INTR_ENABLE);
- pm_imr = intel_uncore_read(uncore,
GEN11_GPM_WGBOXPERF_INTR_MASK);
- /*
-* The equivalent to the PM ISR & IIR cannot be read
-* without affecting the current state of the system
-*/
- pm_isr = 0;
- pm_iir = 0;
- } else if (GRAPHICS_VER(i915) >= 8) {
- pm_ier = intel_uncore_read(uncore, GEN8_GT_IER(2));
- pm_imr = intel_uncore_read(uncore, GEN8_GT_IMR(2));
- pm_isr = intel_uncore_read(uncore, GEN8_GT_ISR(2));
- pm_iir = intel_uncore_read(uncore, GEN8_GT_IIR(2));
- } else {
- pm_ier = intel_uncore_read(uncore, GEN6_PMIER);
- pm_imr = intel_uncore_read(uncore, GEN6_PMIMR);
- pm_isr = intel_uncore_read(uncore, GEN6_PMISR);
- pm_iir = intel_uncore_read(uncore, GEN6_PMIIR);
- }
- pm_mask = intel_uncore_read(uncore, GEN6_PMINTRMSK);
-
- drm_printf(p, "Video Turbo Mode: %s\n",
-
[PATCH v2 0/2] drm/i915/slpc: Update frequency debugfs for SLPC
Remove the RPS related information that is not valid when SLPC is enabled. v2: Add version numbers and address other comments (Jani) Signed-off-by: Vinay Belgaumkar Vinay Belgaumkar (2): drm/i915: Add a wrapper for frequency debugfs drm/i915/slpc: Update the frequency debugfs drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 157 + drivers/gpu/drm/i915/gt/intel_rps.c | 207 ++ drivers/gpu/drm/i915/gt/intel_rps.h | 3 + 3 files changed, 211 insertions(+), 156 deletions(-) -- 2.35.1
Re: [PATCH 10/16] drm/udl: Use damage iterator
On 9/19/22 15:04, Thomas Zimmermann wrote: > Use a damage iterator to process damage areas individually. Merging > damage areas can resul tin large updates of unchanged framebuffer result in > regions. As USB is rather slow, it's better to process damage areas > individually and hence minimize USB-transfered data. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas but I've a comment below. > > /* > @@ -301,16 +291,26 @@ static void > udl_primary_plane_helper_atomic_update(struct drm_plane *plane, > struct drm_shadow_plane_state *shadow_plane_state = > to_drm_shadow_plane_state(plane_state); > struct drm_framebuffer *fb = plane_state->fb; > struct drm_plane_state *old_plane_state = > drm_atomic_get_old_plane_state(state, plane); > - struct drm_rect rect; > - int idx; > + struct drm_atomic_helper_damage_iter iter; > + struct drm_rect damage; > + int ret, idx; > > - if (!drm_dev_enter(dev, )) > + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); > + if (ret) > return; > This is an unrelated change. The sync was made in udl_handle_damage() before and you are moving to udl_primary_plane_helper_atomic_update() in this patch. I don't have a strong opinion and I see that there are drivers that do once before iterating over the damage areas and others do the sync for each damage area update. But it would be good to mention that this change is done too and provided some justification. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 1/2] drm/i915: Add a wrapper for frequency debugfs
On 10/4/2022 12:36 AM, Jani Nikula wrote:
On Mon, 03 Oct 2022, Vinay Belgaumkar wrote:
Move it to the RPS source file.
The idea was that the 1st patch would be non-functional code
movement. This is still a functional change.
Or you can do the functional changes first, and then move code, as long
as you don't combine code movement with functional changes.
Yup, will move the SLPC check to the second patch as well.
Please also mark your patch revisions and note the changes. There's no
indication this series is v2.
ok.
Thanks,
Vinay.
BR,
Jani.
Signed-off-by: Vinay Belgaumkar
---
drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 157 +---
drivers/gpu/drm/i915/gt/intel_rps.c | 169 ++
drivers/gpu/drm/i915/gt/intel_rps.h | 3 +
3 files changed, 173 insertions(+), 156 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
index 9fd4d9255a97..4319d6cdafe2 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
@@ -344,162 +344,7 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
struct drm_printer *p)
drm_printf(p, "efficient (RPe) frequency: %d MHz\n",
intel_gpu_freq(rps, rps->efficient_freq));
} else if (GRAPHICS_VER(i915) >= 6) {
- u32 rp_state_limits;
- u32 gt_perf_status;
- struct intel_rps_freq_caps caps;
- u32 rpmodectl, rpinclimit, rpdeclimit;
- u32 rpstat, cagf, reqf;
- u32 rpcurupei, rpcurup, rpprevup;
- u32 rpcurdownei, rpcurdown, rpprevdown;
- u32 rpupei, rpupt, rpdownei, rpdownt;
- u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
-
- rp_state_limits = intel_uncore_read(uncore,
GEN6_RP_STATE_LIMITS);
- gen6_rps_get_freq_caps(rps, );
- if (IS_GEN9_LP(i915))
- gt_perf_status = intel_uncore_read(uncore,
BXT_GT_PERF_STATUS);
- else
- gt_perf_status = intel_uncore_read(uncore,
GEN6_GT_PERF_STATUS);
-
- /* RPSTAT1 is in the GT power well */
- intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
-
- reqf = intel_uncore_read(uncore, GEN6_RPNSWREQ);
- if (GRAPHICS_VER(i915) >= 9) {
- reqf >>= 23;
- } else {
- reqf &= ~GEN6_TURBO_DISABLE;
- if (IS_HASWELL(i915) || IS_BROADWELL(i915))
- reqf >>= 24;
- else
- reqf >>= 25;
- }
- reqf = intel_gpu_freq(rps, reqf);
-
- rpmodectl = intel_uncore_read(uncore, GEN6_RP_CONTROL);
- rpinclimit = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
- rpdeclimit = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
-
- rpstat = intel_uncore_read(uncore, GEN6_RPSTAT1);
- rpcurupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) &
GEN6_CURICONT_MASK;
- rpcurup = intel_uncore_read(uncore, GEN6_RP_CUR_UP) &
GEN6_CURBSYTAVG_MASK;
- rpprevup = intel_uncore_read(uncore, GEN6_RP_PREV_UP) &
GEN6_CURBSYTAVG_MASK;
- rpcurdownei = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN_EI) &
GEN6_CURIAVG_MASK;
- rpcurdown = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN) &
GEN6_CURBSYTAVG_MASK;
- rpprevdown = intel_uncore_read(uncore, GEN6_RP_PREV_DOWN) &
GEN6_CURBSYTAVG_MASK;
-
- rpupei = intel_uncore_read(uncore, GEN6_RP_UP_EI);
- rpupt = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
-
- rpdownei = intel_uncore_read(uncore, GEN6_RP_DOWN_EI);
- rpdownt = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
-
- cagf = intel_rps_read_actual_frequency(rps);
-
- intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
-
- if (GRAPHICS_VER(i915) >= 11) {
- pm_ier = intel_uncore_read(uncore,
GEN11_GPM_WGBOXPERF_INTR_ENABLE);
- pm_imr = intel_uncore_read(uncore,
GEN11_GPM_WGBOXPERF_INTR_MASK);
- /*
-* The equivalent to the PM ISR & IIR cannot be read
-* without affecting the current state of the system
-*/
- pm_isr = 0;
- pm_iir = 0;
- } else if (GRAPHICS_VER(i915) >= 8) {
- pm_ier = intel_uncore_read(uncore, GEN8_GT_IER(2));
- pm_imr = intel_uncore_read(uncore, GEN8_GT_IMR(2));
- pm_isr = intel_uncore_read(uncore, GEN8_GT_ISR(2));
- pm_iir = intel_uncore_read(uncore, GEN8_GT_IIR(2));
-
Re: [Freedreno] [PATCH 1/5] drm/msm/dsi: Remove useless math in DSC calculation
On 2022-10-04 07:33:49, Abhinav Kumar wrote:
>
>
> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> > Multiplying a value by 2 and adding 1 to it always results in a value
> > that is uneven, and that 1 gets truncated immediately when performing
> > integer division by 2 again. There is no "rounding" possible here.
> >
> > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> > Signed-off-by: Marijn Suijten
> > ---
> > drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +--
> > 1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 8e4bc586c262..e05bae647431 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -1864,12 +1864,7 @@ static int dsi_populate_dsc_params(struct
> > drm_dsc_config *dsc)
> > data = 2048 * (dsc->rc_model_size - dsc->initial_offset +
> > num_extra_mux_bits);
> > dsc->slice_bpg_offset = DIV_ROUND_UP(data, groups_total);
> >
> > - /* bpp * 16 + 0.5 */
> > - data = dsc->bits_per_pixel * 16;
> > - data *= 2;
> > - data++;
> > - data /= 2;
> > - target_bpp_x16 = data;
> > + target_bpp_x16 = dsc->bits_per_pixel * 16;
> >
> Since this patch is titled, "remove useless math", even the
> target_bpp_x16 math looks redundant to me,
>
> first we do
>
> target_bpp_x16 = dsc->bits_per_pixel * 16;
>
> then in the next line we do
>
> data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
>
> the *16 and /16 will cancel out here.
>
> Instead we can just do
>
> data = (dsc->initial_xmit_delay * dsc->drm->bits_per_pixel) ?
Thanks, good catch! I might have been so focused on explaining the
effect of this patch and uselessness of the proposed `+ 0.5` rounding
here that I missed this intermediate variable now becoming redundant as
well.
Corrected for v2!
- Marijn
> > data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
> > final_value = dsc->rc_model_size - data + num_extra_mux_bits;
Re: [PATCH 09/16] drm/udl: Support DRM hot-unplugging
On 9/19/22 15:04, Thomas Zimmermann wrote: > Add drm_dev_enter() and drm_dev_exit() to the various modesetting > functions that interact with the device. After hot-unplugging the > device, these functions will return early. So far, the udl driver > relied on USB interfaces to handle unplugging of the device. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 4/5] drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional bits
On 2022-10-04 10:03:07, Abhinav Kumar wrote:
>
>
> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> > According to the comment this DPU register contains the bits per pixel
> > as a 6.4 fractional value, conveniently matching the contents of
> > bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
> > bits. However, the downstream source this implementation was
> > copy-pasted from has its bpp field stored _without_ fractional part.
> >
> > This makes the entire convoluted math obsolete as it is impossible to
> > pull those 4 fractional bits out of thin air, by somehow trying to reuse
> > the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
> >
> > The rest of the code merely attempts to keep the integer part a multiple
> > of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
> > 12; already filling up those bits anyway (but not on downstream).
> >
> > Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> > Signed-off-by: Marijn Suijten
>
> Many of this bugs are because the downstream code from which this
> implementation was derived wasnt the latest perhaps?
Perhaps, this code is "identical" to what I'm looking at in some
downstream 4.14 / 4.19, where the upstream struct for DSC either wasn't
there or wasn't used. We have to find and address these bugs one by one
to make our panels work, and this series gets one platform (sdm845) down
but has more work pending for others (sm8250 has my current focus).
Or are you suggesting to "redo" the DSC integration work based on a
(much) newer display techpack (SDE driver)?
> Earlier, downstream had its own DSC struct maybe leading to this
> redundant math but now we have migrated over to use the upstream struct
> drm_dsc_config.
Found the 3-year-old `disp: msm: use upstream dsc config data` commit
that makes this change. It carries a similar comment:
/* integer bpp support only */
The superfluous math was howerver removed earlier, in:
disp: msm: fix dsc parameters related to 10 bpc 10 bpp
- Marijn
> That being said, this patch LGTM
> Reviewed-by: Abhinav Kumar
Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields
On 2022-10-04 13:22:25, Abhinav Kumar wrote:
>
> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> > msm's dsi_host specifies negative BPG offsets which fill the full 8 bits
> > of a char thanks to two's complement: this however results in those bits
> > bleeding into the next parameter when the field is only expected to
> > contain 6-bit wide values.
> > As a consequence random slices appear corrupted on-screen (tested on a
> > Sony Tama Akatsuki device with sdm845).
> >
> > Use AND operators to limit all values that constitute the RC Range
> > parameter fields to their expected size.
> >
> > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> > Signed-off-by: Marijn Suijten
> > ---
> > drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c
> > b/drivers/gpu/drm/display/drm_dsc_helper.c
> > index c869c6e51e2b..2e7ef242685d 100644
> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> > @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct
> > drm_dsc_picture_parameter_set *pps_payload,
> > */
> > for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> > pps_payload->rc_range_parameters[i] =
> > - cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
> > + cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp
> > & 0x1f) <<
> > DSC_PPS_RC_RANGE_MINQP_SHIFT) |
> > - (dsc_cfg->rc_range_params[i].range_max_qp <<
> > + ((dsc_cfg->rc_range_params[i].range_max_qp
> > & 0x1f) <<
> > DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
> > -
> > (dsc_cfg->rc_range_params[i].range_bpg_offset));
> > +
> > (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
> > }
> >
>
> Looking at some examples of this for other vendors, they have managed to
> limit the value to 6 bits in their drivers:
>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L532
>
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dsc/rc_calc_dpi.c#L87
>
> Perhaps, msm should do the same thing instead of the helper change.
Thanks, I should have done my due-diligence and look up how other
drivers dealt with this, but wasn't immediately expecting negative
values elsewhere.
Alas, as explained in the cover letter I opted to perform the masking in
the PPS packing code as the DSC block code also reads these values, and
would suddenly write 6-bit intead of 8-bit values to the
DSC_RANGE_BPG_OFFSET registers. Quick testing on the mentioned sdm845
platform shows no regressions, but I'm not sure if that's safe to rely
on?
> If you want to move to helper, other drivers need to be changed too to
> remove duplicate & 0x3f.
Sure, we only have to confirm whether those drivers also read back the
value(s) in rc_range_params, and expect / allow this to be 8 instead of
6 bits.
> FWIW, this too has already been fixed in the latest downstream driver too.
What is this supposed to mean? Is there a downstream DPU project that
has pending patches needing to be upstreamed? Or is the downstream SDE,
techpack/display, or whatever it is called nowadays, slowly using more
DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack()
helper function as pointed out in an earlier mail?
Offtopic: are SDE and DPU growing closer together, hopefully achieving
feature parity allowing the SDE project to be dropped in favour of a
fully upstreamed DPU driver for day-one out-of-the-box mainline support
for new SoCs (as long as work is published and on its way upstream)?
- Marijn
Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields
On 2022-10-04 17:41:07, Dmitry Baryshkov wrote: > On Sat, 1 Oct 2022 at 23:23, Marijn Suijten > wrote: > [..] > > Pre-empting the reviews: I was contemplating whether to use FIELD_PREP > > here, given that it's not yet used anywhere else in this file. For that > > I'd remove the existing _SHIFT definitions and replace them with: > > > > #define DSC_PPS_RC_RANGE_MINQP_MASK GENMASK(15, 11) > > #define DSC_PPS_RC_RANGE_MAXQP_MASK GENMASK(10, 6) > > #define DSC_PPS_RC_RANGE_BPG_OFFSET_MASKGENMASK(5, 0) > > > > And turn this section of code into: > > > > cpu_to_be16(FIELD_PREP(DSC_PPS_RC_RANGE_MINQP_MASK, > >dsc_cfg->rc_range_params[i].range_min_qp) | > > FIELD_PREP(DSC_PPS_RC_RANGE_MAXQP_MASK, > >dsc_cfg->rc_range_params[i].range_max_qp) | > > FIELD_PREP(DSC_PPS_RC_RANGE_BPG_OFFSET_MASK, > > > > dsc_cfg->rc_range_params[i].range_bpg_offset)); > > > > Is that okay/recommended? > > This is definitely easier to review. However if you do not want to use > FIELD_PREP, it would be better to split this into a series of `data |= > something` assignments terminated with the rc_range_parameters[i] > assignment. Anything is fine by me, I have no strong opinion on this and rather leave it up to the maintainers. However, FIELD_PREP gives us concise `#define`s through a single `GENMASK()` carrying both the shift and mask/field-width. At the same time these genmask definitions map more clearly to the layout comment right above: /* * For DSC sink programming the RC Range parameter fields * are as follows: Min_qp[15:11], max_qp[10:6], offset[5:0] */ If switching to `data |=` however, I've been recommended to not use FIELD_PREP but fulyl write out `data |= (value & MASK) << SHIFT` instead. Perhaps a more important question is how to apply this consistently throughout the function? - Marijn
[RFC PATCH 5/5] drm/amd/display: Fill 3D LUT from userspace
Implement the 3D LUT interface, convert and pass the data for amdgpu
driver.
Note: A patchset "IGT tests for pre-blending 3D LUT interfaces" for this
proposal is sent to IGT mailing list.
Signed-off-by: Alex Hung
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 ++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
.../amd/display/amdgpu_dm/amdgpu_dm_color.c | 181 ++
3 files changed, 195 insertions(+)
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 7094578a683f..10e6dc5c8552 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5656,6 +5656,19 @@ static int fill_dc_plane_attributes(struct amdgpu_device
*adev,
dc_plane_state->in_transfer_func->type = TF_TYPE_HWPWL;
}
+ /* 3D LUT from userspace */
+ if (plane_state->color_mgmt_changed) {
+ if (plane_state->lut_3d && dc_plane_state->lut3d_func) {
+ ret = amdgpu_dm_fill_3dlut_data(plane_state,
_plane_state->lut3d_func->lut_3d);
+ if (!ret)
+
dc_plane_state->lut3d_func->state.bits.initialized = 1;
+ else
+ return ret;
+ } else {
+ /* TODO disable 3D LUT */
+ }
+ }
+
return 0;
}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 667957087ccf..644c5ff6ee9a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -726,6 +726,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state
*crtc,
struct dc_plane_state *dc_plane_state);
void amdgpu_dm_fill_pwl_data(struct drm_property_blob *lut_blob, struct
pwl_params *lut_params, struct drm_color_lut_range *pwl_definition, int
pwl_size);
+int amdgpu_dm_fill_3dlut_data(const struct drm_plane_state *plane_state,
struct tetrahedral_params *param);
void amdgpu_dm_update_connector_after_detect(
struct amdgpu_dm_connector *aconnector);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index ae633fe52525..705852bf63e7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -22,6 +22,7 @@
* Authors: AMD
*
*/
+ #include
#include "amdgpu.h"
#include "amdgpu_mode.h"
#include "amdgpu_dm.h"
@@ -469,6 +470,186 @@ int amdgpu_dm_verify_lut_sizes(const struct
drm_crtc_state *crtc_state)
return 0;
}
+#define R_3DLUT0
+#define G_3DLUT1
+#define B_3DLUT2
+
+static __u16 extract_rgb_value(void *lut_3d, __u32 color_format, __u8 color)
+{
+ __u64 val = *(__u64 *) lut_3d;
+
+ switch (color_format) {
+ case DRM_FORMAT_XRGB16161616:
+ if (color == R_3DLUT)
+ return val & 0x;
+ else if (color == G_3DLUT)
+ return (val >> 16) & 0x;
+ else if (color == B_3DLUT)
+ return (val >> 32) & 0x;
+ break;
+ case DRM_FORMAT_XBGR16161616:
+ if (color == B_3DLUT)
+ return val & 0x;
+ else if (color == G_3DLUT)
+ return (val >> 16) & 0x;
+ else if (color == R_3DLUT)
+ return (val >> 32) & 0x;
+ break;
+ case DRM_FORMAT_XRGB:
+ if (color == R_3DLUT)
+ return val & 0xFF;
+ else if (color == G_3DLUT)
+ return (val >> 8) & 0xFF;
+ else if (color == B_3DLUT)
+ return (val >> 16) & 0xFF;
+ break;
+ case DRM_FORMAT_XBGR:
+ if (color == B_3DLUT)
+ return val & 0xFF;
+ else if (color == G_3DLUT)
+ return (val >> 8) & 0xFF;
+ else if (color == R_3DLUT)
+ return (val >> 16) & 0xFF;
+ break;
+ default:
+ return 0;
+ }
+
+ return 0;
+}
+
+static bool extract_rgb_data(const struct drm_plane_state *plane_state, struct
drm_mode_3dlut_mode *mode, __u16 *lut_data)
+{
+ __u16 i, lut_volume;
+ void *lut_3d = plane_state->lut_3d->data;
+ __u32 cfmt = mode->color_format;
+
+ /* copy RGB accordingly */
+ lut_volume = mode->lut_size * mode->lut_size * mode->lut_size;
+ for (i = 0; i < lut_volume; i += 3) {
+ lut_data[i] = extract_rgb_value(lut_3d, cfmt, R_3DLUT);
+ lut_data[i+1] = extract_rgb_value(lut_3d, cfmt,
[RFC PATCH 4/5] drm/amd/display: Enable plane 3DLUT mode
Enable the 3D LUT mode supported by amdgpu.
Note: A patchset "IGT tests for pre-blending 3D LUT interfaces" for this
proposal is sent to IGT mailing list.
Signed-off-by: Alex Hung
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++
drivers/gpu/drm/drm_color_mgmt.c | 31 +++
include/drm/drm_plane.h | 2 ++
3 files changed, 36 insertions(+)
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 ee277f357140..7094578a683f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8008,6 +8008,9 @@ static int amdgpu_dm_plane_init(struct
amdgpu_display_manager *dm,
/* TODO need to check ASICs */
drm_plane_create_3d_lut_properties(plane->dev, plane, 1);
+ res = drm_plane_color_add_3dlut_mode(plane, "3dlut_17_12bit",
_3d_mode_17_12bit, sizeof(lut_3d_mode_17_12bit));
+ if (res)
+ return res;
drm_plane_attach_3dlut_properties(plane);
/* Create (reset) the plane state */
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 4bfe5b5c9670..5418ca24db73 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -743,6 +743,37 @@ void drm_plane_attach_3dlut_properties(struct drm_plane
*plane)
}
EXPORT_SYMBOL(drm_plane_attach_3dlut_properties);
+int drm_plane_color_add_3dlut_mode(struct drm_plane *plane,
+const char *name,
+const struct
drm_mode_3dlut_mode *mode_3dlut,
+size_t length)
+{
+ struct drm_property_blob *blob;
+ struct drm_property *prop = NULL;
+ int ret;
+
+ prop = plane->lut_3d_mode_property;
+
+ if (!prop)
+ return -EINVAL;
+
+ if (length == 0 && name)
+ return drm_property_add_enum(prop, 0, name);
+
+ blob = drm_property_create_blob(plane->dev, length, mode_3dlut);
+ if (IS_ERR(blob))
+ return PTR_ERR(blob);
+
+ ret = drm_property_add_enum(prop, blob->base.id, name);
+ if (ret) {
+ drm_property_blob_put(blob);
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_plane_color_add_3dlut_mode);
+
int drm_plane_color_add_gamma_degamma_mode_range(struct drm_plane *plane,
const char *name,
const struct
drm_color_lut_range *ranges,
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 4e272144170f..f94f91466675 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -946,6 +946,8 @@ int drm_plane_create_3d_lut_properties(struct drm_device
*dev,
struct drm_plane *plane,
int num_values);
void drm_plane_attach_3dlut_properties(struct drm_plane *plane);
+int drm_plane_color_add_3dlut_mode(struct drm_plane *plane, const char *name,
+const struct
drm_mode_3dlut_mode *mode_3dlut, size_t length);
int drm_plane_color_add_gamma_degamma_mode_range(struct drm_plane *plane,
const char *name,
const struct
drm_color_lut_range *ranges,
--
2.37.3
[RFC PATCH 3/5] drm/amd/display: Define 3D LUT struct for HDR planes
Add a 3D LUT mode supported by amdgpu driver.
Note: A patchset "IGT tests for pre-blending 3D LUT interfaces" for this
proposal is sent to IGT mailing list.
Signed-off-by: Alex Hung
---
.../gpu/drm/amd/display/modules/color/color_gamma.h | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/amd/display/modules/color/color_gamma.h
b/drivers/gpu/drm/amd/display/modules/color/color_gamma.h
index e06e0a8effc8..aceb23b03a4b 100644
--- a/drivers/gpu/drm/amd/display/modules/color/color_gamma.h
+++ b/drivers/gpu/drm/amd/display/modules/color/color_gamma.h
@@ -27,6 +27,7 @@
#define COLOR_MOD_COLOR_GAMMA_H_
#include "color_table.h"
+#include
struct dc_transfer_func;
struct dc_gamma;
@@ -35,6 +36,17 @@ struct dc_rgb_fixed;
struct dc_color_caps;
enum dc_transfer_func_predefined;
+/*
+ * 3D LUT mode for 17x17x17 LUT and 12 bits of color depth
+ */
+static const struct drm_mode_3dlut_mode lut_3d_mode_17_12bit = {
+ .lut_size = 17,
+ .lut_stride = {17, 17, 18},
+ .bit_depth = 12,
+ .color_format = DRM_FORMAT_XRGB16161616,
+ .flags = 0,
+};
+
static const struct drm_color_lut_range nonlinear_pwl[] = {
{
.flags = (DRM_MODE_LUT_GAMMA | DRM_MODE_LUT_REFLECT_NEGATIVE |
DRM_MODE_LUT_INTERPOLATE | DRM_MODE_LUT_REUSE_LAST |
DRM_MODE_LUT_NON_DECREASING),
--
2.37.3
[RFC PATCH 2/5] drm: Add Plane 3DLUT and 3DLUT mode properties
Add plane lut_3d mode and lut_3d as blob properties.
lut_3d mode is an enum property with values as blob_ids.
Userspace can get supported modes and also set one of the modes.
Note: A patchset "IGT tests for pre-blending 3D LUT interfaces" for this
proposal is sent to IGT mailing list.
Signed-off-by: Alex Hung
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++
drivers/gpu/drm/drm_atomic_state_helper.c | 3 ++
drivers/gpu/drm/drm_atomic_uapi.c | 11 ++
drivers/gpu/drm/drm_color_mgmt.c | 37 +++
include/drm/drm_mode_object.h | 2 +-
include/drm/drm_plane.h | 31
6 files changed, 87 insertions(+), 1 deletion(-)
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 f546c1326db3..ee277f357140 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8006,6 +8006,10 @@ static int amdgpu_dm_plane_init(struct
amdgpu_display_manager *dm,
drm_plane_attach_gamma_properties(plane);
drm_plane_attach_ctm_property(plane);
+ /* TODO need to check ASICs */
+ drm_plane_create_3d_lut_properties(plane->dev, plane, 1);
+ drm_plane_attach_3dlut_properties(plane);
+
/* Create (reset) the plane state */
if (plane->funcs->reset)
plane->funcs->reset(plane);
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 7ddf6e4b956b..85900cd1bffe 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -318,6 +318,8 @@ void __drm_atomic_helper_plane_duplicate_state(struct
drm_plane *plane,
drm_property_blob_get(state->ctm);
if (state->gamma_lut)
drm_property_blob_get(state->gamma_lut);
+ if (state->lut_3d)
+ drm_property_blob_get(state->lut_3d);
state->color_mgmt_changed = false;
}
@@ -369,6 +371,7 @@ void __drm_atomic_helper_plane_destroy_state(struct
drm_plane_state *state)
drm_property_blob_put(state->degamma_lut);
drm_property_blob_put(state->ctm);
drm_property_blob_put(state->gamma_lut);
+ drm_property_blob_put(state->lut_3d);
}
EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
b/drivers/gpu/drm/drm_atomic_uapi.c
index ba3e64cb184a..66e59e7c194d 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -622,6 +622,13 @@ static int drm_atomic_plane_set_property(struct drm_plane
*plane,
);
state->color_mgmt_changed |= replaced;
return ret;
+ } else if (property == plane->lut_3d_property) {
+ ret = drm_atomic_replace_property_blob_from_id(dev,
+ >lut_3d, val, -1, 8, );
+ state->color_mgmt_changed |= replaced;
+ return 0;
+ } else if (property == plane->lut_3d_mode_property) {
+ state->lut_3d_mode = val;
} else if (property == config->prop_fb_damage_clips) {
ret = drm_atomic_replace_property_blob_from_id(dev,
>fb_damage_clips,
@@ -700,6 +707,10 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
} else if (property == plane->gamma_lut_property) {
*val = (state->gamma_lut) ?
state->gamma_lut->base.id : 0;
+ } else if (property == plane->lut_3d_property) {
+ *val = (state->lut_3d) ? state->lut_3d->base.id : 0;
+ } else if (property == plane->lut_3d_mode_property) {
+ *val = state->lut_3d_mode;
} else if (property == config->prop_fb_damage_clips) {
*val = (state->fb_damage_clips) ?
state->fb_damage_clips->base.id : 0;
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index b5b3ff7f654d..4bfe5b5c9670 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -706,6 +706,43 @@ void drm_plane_attach_gamma_properties(struct drm_plane
*plane)
}
EXPORT_SYMBOL(drm_plane_attach_gamma_properties);
+int drm_plane_create_3d_lut_properties(struct drm_device *dev,
+ struct drm_plane *plane,
+ int num_values)
+{
+ struct drm_property *mode;
+ struct drm_property *blob;
+
+ mode = drm_property_create(dev, DRM_MODE_PROP_ENUM,
"PLANE_3D_LUT_MODE", num_values);
+ if (!mode)
+ return -ENOMEM;
+
+ plane->lut_3d_mode_property = mode;
+
+ blob = drm_property_create(dev, DRM_MODE_PROP_BLOB, "PLANE_3D_LUT", 0);
+ if (!blob)
+ return -ENOMEM;
+
+
[RFC PATCH 1/5] drm: Add 3D LUT mode and its attributes
A struct is defined for 3D LUT modes to be supported by hardware.
The elements includes lut_isze, lut_stride, bit_depth, color_format
and flags.
Note: A patchset "IGT tests for pre-blending 3D LUT interfaces" for this
proposal is sent to IGT mailing list.
Signed-off-by: Alex Hung
---
include/uapi/drm/drm_mode.h | 17 +
1 file changed, 17 insertions(+)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index d0ce48d2e732..334e8a9b49cc 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -877,6 +877,23 @@ struct drm_color_lut_ext {
__u64 reserved;
};
+/*
+ * struct drm_mode_3dlut_mode - 3D LUT mode information.
+ * @lut_size: number of valid points on every dimension of 3D LUT.
+ * @lut_stride: number of points on every dimension of 3D LUT.
+ * @bit_depth: number of bits of RGB. If color_mode defines entries with higher
+ * bit_depth the least significant bits will be truncated.
+ * @color_format: fourcc values, ex. DRM_FORMAT_XRGB16161616 or
DRM_FORMAT_XBGR16161616.
+ * @flags: flags for hardware-sepcific features
+ */
+struct drm_mode_3dlut_mode {
+ __u16 lut_size;
+ __u16 lut_stride[3];
+ __u16 bit_depth;
+ __u32 color_format;
+ __u32 flags;
+};
+
#define DRM_MODE_PAGE_FLIP_EVENT 0x01
#define DRM_MODE_PAGE_FLIP_ASYNC 0x02
#define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
--
2.37.3
[RFC PATCH 0/5] Proposal for Pre-blending 3D LUT interfaces
This is an proposal and a draft implementation to enable 3D LUT on
drm_plane. This proposal defines a new interface for userspace
applications to query hardware capabilities and to pass/enable 3D LUT
functions via this DRM/KMS APIs.
Overviews:
┌─┐┌─┐┌───┐┌──┐ ┌┐
│Userspace│◄──►│3DLUT API│◄──►│DRM│◄──►│GPU driver├──►│hardware│
└─┘└─┘└───┘└──┘ └┘
1. Userspace queries the 3DLUT mode (defined by drm_mode_3dlut_mode)
from the GPU drivers (ex. amdgpu).
2. The GPU Driver replies sizes and the color depth of the
3DLUT modes, such as defined by struct lut_3d_mode_17_12bit.
3. If applicable, userspace selects and sets one of preferred 3DLUT
modes by "lut_3d_mode" to driver.
4. Userspace passes 3D LUT via drm_property_blob "lut_3d". In the case
of the mode "lut_3d_mode_17_12bit", the 3D LUT should have a cube
size = 17x17x17 (lut_size), color depth = 12 bits (bit_depth), and
X/Y/Z axis = R/G/B (color_format).
5. The GPU driver parses 3D LUT and passes it to hardware pipeline, and
enables 3D LUT accordingly.
Notes:
1. The patchset is based on the previous work on
https://gitlab.freedesktop.org/hwentland/linux/-/tree/color-and-hdr
2. This interface can be part of the newly proposed DRM/KMS color pipeline
API (https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/11). A
future integration to the new API may be required or preferred, such
as the followings:
struct drm_color_pipeline_element {
drm_color_pipeline_element_type;
drm_color_pipeline_element_lut3d
};
struct drm_mode_3dlut_mode -> struct drm_color_pipeline_lut3d_config
struct drm_color_pipeline_lut3d {
lut_3d_mode_17_12bit;
};
struct drm_color_pipeline_lut3d_data {
*lut_3d;
};
and etc.
3. A patchset "IGT tests for pre-blending 3D LUT interfaces" for this
proposal is sent to IGT mailing list.
Related Work:
- Enable 3D LUT to AMD display drivers
(https://www.spinics.net/lists/amd-gfx/msg83032.html)
Alex Hung (5):
drm: Add 3D LUT mode and its attributes
drm: Add Plane 3DLUT and 3DLUT mode properties
drm/amd/display: Define 3D LUT struct for HDR planes
drm/amd/display: Enable plane 3DLUT mode
drm/amd/display: Fill 3D LUT from userspace
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
.../amd/display/amdgpu_dm/amdgpu_dm_color.c | 181 ++
.../amd/display/modules/color/color_gamma.h | 12 ++
drivers/gpu/drm/drm_atomic_state_helper.c | 3 +
drivers/gpu/drm/drm_atomic_uapi.c | 11 ++
drivers/gpu/drm/drm_color_mgmt.c | 68 +++
include/drm/drm_mode_object.h | 2 +-
include/drm/drm_plane.h | 33
include/uapi/drm/drm_mode.h | 17 ++
10 files changed, 347 insertions(+), 1 deletion(-)
--
2.37.3
Re: [PATCH v6 01/10] drm: bridge: Add Samsung DSIM bridge driver
On Mon, Oct 3, 2022 at 1:23 PM Marek Szyprowski
wrote:
>
> Hi Jagan,
>
> On 01.10.2022 10:06, Jagan Teki wrote:
> > Samsung MIPI DSIM controller is common DSI IP that can be used in various
> > SoCs like Exynos, i.MX8M Mini/Nano.
> >
> > In order to access this DSI controller between various platform SoCs,
> > the ideal way to incorporate this in the drm stack is via the drm bridge
> > driver.
> >
> > This patch is trying to differentiate platform-specific and bridge driver
> > code by maintaining exynos platform glue code in exynos_drm_dsi.c driver
> > and common bridge driver code in samsung-dsim.c providing that the new
> > platform-specific glue should be supported in the bridge driver, unlike
> > exynos platform drm drivers.
> >
> > - Add samsung_dsim_plat_data for keeping platform-specific attributes like
> >host_ops, irq_ops, and hw_type.
> >
> > - Initialize the plat_data hooks for exynos platform in exynos_drm_dsi.c.
> >
> > - samsung_dsim_probe is the common probe call across exynos_drm_dsi.c and
> >samsung-dsim.c.
> >
> > - plat_data hooks like host_ops and irq_ops are invoked during the
> >respective bridge call chains.
> >
> > v6:
> > * handle previous bridge pointer for exynos dsi
>
> There are still issues, see my comments below.
>
> > v5:
> > * [mszyprow] reworked driver initialization using the same approach as in
> >drivers/gpu/drm/{exynos/exynos_dp.c, bridge/analogix/analogix_dp_core.c},
> >removed weak functions, moved exynos_dsi_driver back to exynos_drm_dsi.c
> >and restored integration with exynos_drm custom initialization.
> > * updated the commit message [Jagan]
> >
> > v4:
> > * include Inki Dae in MAINTAINERS
> > * remove dsi_driver probe in exynos_drm_drv to support multi-arch build
> >
> > v3:
> > * restore gpio related fixes
> > * restore proper bridge chain
> > * rework initialization issue
> > * fix header includes in proper way
> >
> > v2:
> > * fixed exynos dsi driver conversion (Marek Szyprowski)
> > * updated commit message
> > * updated MAINTAINERS file
< snip>
> > + /**
> > + * FIXME:
> > + * This has to remove once the bridge chain order is compatible
> > + * with Exynos DSI drivers.
> > + *
> > + * DRM bridge chain ordering is not compatible with Exynos DSI
> > + * drivers because DSI sink devices typically want the DSI host
> > + * powered up and configured before they are powered up.
> > + *
> > + * Passing NULL to the previous bridge makes Exynos DSI drivers
> > + * work which is exactly done before.
> > + */
> > + if (!(dsi->plat_data->hw_type & SAMSUNG_DSIM_TYPE_IMX8MM))
>
> The above should be 'if (!(dsi->plat_data->hw_type ==
> SAMSUNG_DSIM_TYPE_IMX8MM))', hw_type is not a bitmask. Also
> SAMSUNG_DSIM_TYPE_IMX8MM is not yet defined in this patch, so this it breaks
> builds.
I thought I fixed it in previous versions. I will fix this in the next version.
>
> > + previous = NULL;
> > +
> > + return drm_bridge_attach(bridge->encoder, dsi->out_bridge, previous,
> > + flags);
> > +}
> > +
> > +static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
> > + .atomic_duplicate_state =
> > drm_atomic_helper_bridge_duplicate_state,
> > + .atomic_destroy_state =
> > drm_atomic_helper_bridge_destroy_state,
> > + .atomic_reset = drm_atomic_helper_bridge_reset,
> > + .atomic_pre_enable = samsung_dsim_atomic_pre_enable,
> > + .atomic_enable = samsung_dsim_atomic_enable,
> > + .atomic_disable = samsung_dsim_atomic_disable,
> > + .atomic_post_disable= samsung_dsim_atomic_post_disable,
> > + .mode_set = samsung_dsim_mode_set,
> > + .attach = samsung_dsim_attach,
> > +};
> > +
> > +static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > + struct mipi_dsi_device *device)
> > +{
> > + struct samsung_dsim *dsi = host_to_dsi(host);
> > + const struct samsung_dsim_plat_data *pdata = dsi->plat_data;
> > + struct device *dev = dsi->dev;
> > + struct drm_panel *panel;
> > + int ret;
> > +
> > + panel = of_drm_find_panel(device->dev.of_node);
> > + if (!IS_ERR(panel)) {
> > + dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > + } else {
> > + dsi->out_bridge = of_drm_find_bridge(device->dev.of_node);
> > + if (!dsi->out_bridge)
> > + dsi->out_bridge = ERR_PTR(-EINVAL);
> > + }
> > +
> > + if (IS_ERR(dsi->out_bridge)) {
> > + ret = PTR_ERR(dsi->out_bridge);
> > + DRM_DEV_ERROR(dev, "failed to find the bridge: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + DRM_DEV_INFO(dev, "Attached %s device\n", device->name);
> > +
> > + drm_bridge_add(>bridge);
> > +
> > + if
Re: [PATCH] dt-bindings: display: panel: use spi-peripheral-props.yaml
On Tue, Oct 4, 2022 at 8:17 AM Laurent Pinchart
wrote:
>
> Hi Krzysztof,
>
> On Tue, Oct 04, 2022 at 03:10:29PM +0200, Krzysztof Kozlowski wrote:
> > On 04/10/2022 15:03, Laurent Pinchart wrote:
> > > On Tue, Oct 04, 2022 at 02:09:07PM +0200, Krzysztof Kozlowski wrote:
> > >> For devices connectable by SPI bus (e.g. already using
> > >> "spi-max-frequency" property), reference the "spi-peripheral-props.yaml"
> > >> schema to allow using all SPI device properties, even these which device
> > >> bindings author did not tried yet.
> > >
> > > Isn't this done implicitly by spi-controller.yaml ? SPI devices that are
> > > children of an SPI controller should match the patternProperties
> > > "^.*@[0-9a-f]+$" in that file, which has a $ref:
> > > spi-peripheral-props.yaml.
> > > Is there something I'm missing ?
> >
> > You are correct about one side of this - SPI controller bindings.
> > However these schemas here have clear: additional/unevaluatedProperties:
> > false, thus when they find DTS like:
> > panel@xxx {
> > compatible = "one of these spi panels";
> > ...
> > spi-cs-high;
> > spi-rx-delay-us = <50>;
> > ... and some more from specific controllers
> > }
> >
> > you will get errors, because the panel schema does not allow them.
> >
> > The bindings were done (some time ago) in such way, that they require
> > that both SPI controller and SPI device reference spi-props.
>
> You're absolutely right that additionalProperties needs to be replaced
> by unevaluatedProperties. Can the additions of $ref be dropped, or is
> that needed too ?
unevaluatedProperties doesn't work with child node schemas (from one
or both schemas). This is because the schemas are applied
independently and can't 'see' each other. The spi-controller.yaml
schema is applied to the SPI bus node and SPI peripheral schemas are
applied to SPI device nodes. This means that child node schemas have
to either be complete or only list properties which will be listed in
the complete schema for the child nodes. For example, 'reg' has to be
listed anyways to define how many entries. This is also why we need a
ref at each level in the graph binding anytime there are additional
properties defined.
Rob
Re: [PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of drm_dp_add_payload_part2()
On 2022-10-04 16:24, Lyude Paul wrote:
Yikes, it appears somehow I totally made a mistake here. We're currently
checking to see if drm_dp_add_payload_part2() returns a non-zero value to
indicate success. That's totally wrong though, as this function only
returns a zero value on success - not the other way around.
So, fix that.
Signed-off-by: Lyude Paul
Issue: https://gitlab.freedesktop.org/drm/amd/-/issues/2171
Fixes: 4d07b0bc4034 ("drm/display/dp_mst: Move all payload info into the atomic
state")
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index b8077fcd4651..00598def5b39 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -297,7 +297,7 @@ bool dm_helpers_dp_mst_send_payload_allocation(
clr_flag = MST_ALLOCATE_NEW_PAYLOAD;
}
- if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state->base.state, payload)) {
+ if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
payload) == 0) {
amdgpu_dm_set_mst_status(>mst_status,
set_flag, false);
} else {
Hi Lyude,
Maybe I'm missing something, but I can't find the
drm_dp_add_payload_part2() function on amd-staging-drm-next. Which repo
are you using?
Thanks
Siqueira
Re: [RFC/PATCH] backlight: hx8357: prepare to conversion to gpiod API
On Tue, Oct 04, 2022 at 09:50:25PM +0200, Linus Walleij wrote: > On Tue, Oct 4, 2022 at 2:54 PM Daniel Thompson > wrote: > > > > We need to know if i.MX is shipping device trees stored in flash, > > > or if they bundle it with the kernel. > > > > This part is frequently found in add-on boards so it's not purely an > > i.MX-only question. > > Oh > > > IMHO for not-in-the-soc devices like this the presence of in-kernel DTs > > isn't enough to make a decision. What is needed is a degree of > > due-diligence to show that there are no obvious out-of-kernel users. > > OK I think this is a good case to use a special quirk in this case. > I actually envisioned keeping piling them up, and that they would > not be innumerable. > > Dmitry, could you fix this? Just patch away in gpiolib-of.c. Sure, I'll add a few quirks. I wonder what is the best way to merge this? I can create a bunch of IBs to be pulled, or I can send quirks to you/Bartosz and once they land send the patches to drivers... Thanks. -- Dmitry
[PATCH] drm/amdgpu/dm/mst: Fix incorrect usage of drm_dp_add_payload_part2()
Yikes, it appears somehow I totally made a mistake here. We're currently
checking to see if drm_dp_add_payload_part2() returns a non-zero value to
indicate success. That's totally wrong though, as this function only
returns a zero value on success - not the other way around.
So, fix that.
Signed-off-by: Lyude Paul
Issue: https://gitlab.freedesktop.org/drm/amd/-/issues/2171
Fixes: 4d07b0bc4034 ("drm/display/dp_mst: Move all payload info into the atomic
state")
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index b8077fcd4651..00598def5b39 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -297,7 +297,7 @@ bool dm_helpers_dp_mst_send_payload_allocation(
clr_flag = MST_ALLOCATE_NEW_PAYLOAD;
}
- if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
payload)) {
+ if (enable && drm_dp_add_payload_part2(mst_mgr, mst_state->base.state,
payload) == 0) {
amdgpu_dm_set_mst_status(>mst_status,
set_flag, false);
} else {
--
2.37.3
Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields
On 10/1/2022 12:08 PM, Marijn Suijten wrote:
msm's dsi_host specifies negative BPG offsets which fill the full 8 bits
of a char thanks to two's complement: this however results in those bits
bleeding into the next parameter when the field is only expected to
contain 6-bit wide values.
As a consequence random slices appear corrupted on-screen (tested on a
Sony Tama Akatsuki device with sdm845).
Use AND operators to limit all values that constitute the RC Range
parameter fields to their expected size.
Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c
b/drivers/gpu/drm/display/drm_dsc_helper.c
index c869c6e51e2b..2e7ef242685d 100644
--- a/drivers/gpu/drm/display/drm_dsc_helper.c
+++ b/drivers/gpu/drm/display/drm_dsc_helper.c
@@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct
drm_dsc_picture_parameter_set *pps_payload,
*/
for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
pps_payload->rc_range_parameters[i] =
- cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
+ cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp &
0x1f) <<
DSC_PPS_RC_RANGE_MINQP_SHIFT) |
- (dsc_cfg->rc_range_params[i].range_max_qp <<
+ ((dsc_cfg->rc_range_params[i].range_max_qp &
0x1f) <<
DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
-
(dsc_cfg->rc_range_params[i].range_bpg_offset));
+ (dsc_cfg->rc_range_params[i].range_bpg_offset
& 0x3f));
}
Looking at some examples of this for other vendors, they have managed to
limit the value to 6 bits in their drivers:
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L532
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dsc/rc_calc_dpi.c#L87
Perhaps, msm should do the same thing instead of the helper change.
If you want to move to helper, other drivers need to be changed too to
remove duplicate & 0x3f.
FWIW, this too has already been fixed in the latest downstream driver too.
Thanks
Abhinav
/* PPS 88 */
Re: [PATCH v2 15/17] drm/i915/vm_bind: Handle persistent vmas in execbuf3
On Mon, Oct 03, 2022 at 01:18:27PM +0100, Matthew Auld wrote:
On 03/10/2022 07:12, Niranjana Vishwanathapura wrote:
Handle persistent (VM_BIND) mappings during the request submission
in the execbuf3 path.
v2: Ensure requests wait for bindings to complete.
Signed-off-by: Niranjana Vishwanathapura
Signed-off-by: Andi Shyti
---
.../gpu/drm/i915/gem/i915_gem_execbuffer3.c | 214 +-
1 file changed, 213 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
index 66b723842e45..5cece16949d8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
@@ -19,6 +19,7 @@
#include "i915_gem_vm_bind.h"
#include "i915_trace.h"
+#define __EXEC3_HAS_PINBIT_ULL(33)
#define __EXEC3_ENGINE_PINNED BIT_ULL(32)
#define __EXEC3_INTERNAL_FLAGS (~0ull << 32)
@@ -42,7 +43,9 @@
* execlist. Hence, no support for implicit sync.
*
* The new execbuf3 ioctl only works in VM_BIND mode and the VM_BIND mode only
- * works with execbuf3 ioctl for submission.
+ * works with execbuf3 ioctl for submission. All BOs mapped on that VM (through
+ * VM_BIND call) at the time of execbuf3 call are deemed required for that
+ * submission.
*
* The execbuf3 ioctl directly specifies the batch addresses instead of as
* object handles as in execbuf2 ioctl. The execbuf3 ioctl will also not
@@ -58,6 +61,13 @@
* So, a lot of code supporting execbuf2 ioctl, like relocations, VA evictions,
* vma lookup table, implicit sync, vma active reference tracking etc., are not
* applicable for execbuf3 ioctl.
+ *
+ * During each execbuf submission, request fence is added to all VM_BIND mapped
+ * objects with DMA_RESV_USAGE_BOOKKEEP. The DMA_RESV_USAGE_BOOKKEEP usage will
+ * prevent over sync (See enum dma_resv_usage). Note that DRM_I915_GEM_WAIT and
+ * DRM_I915_GEM_BUSY ioctls do not check for DMA_RESV_USAGE_BOOKKEEP usage and
+ * hence should not be used for end of batch check. Instead, the execbuf3
+ * timeline out fence should be used for end of batch check.
*/
/**
@@ -127,6 +137,23 @@ eb_find_vma(struct i915_address_space *vm, u64 addr)
return i915_gem_vm_bind_lookup_vma(vm, va);
}
+static void eb_scoop_unbound_vma_all(struct i915_address_space *vm)
+{
+ struct i915_vma *vma, *vn;
+
+ /**
+* Move all unbound vmas back into vm_bind_list so that they are
+* revalidated.
+*/
+ spin_lock(>vm_rebind_lock);
+ list_for_each_entry_safe(vma, vn, >vm_rebind_list, vm_rebind_link) {
+ list_del_init(>vm_rebind_link);
+ if (!list_empty(>vm_bind_link))
+ list_move_tail(>vm_bind_link, >vm_bind_list);
+ }
+ spin_unlock(>vm_rebind_lock);
+}
+
static int eb_lookup_vma_all(struct i915_execbuffer *eb)
{
unsigned int i, current_batch = 0;
@@ -141,14 +168,121 @@ static int eb_lookup_vma_all(struct i915_execbuffer *eb)
++current_batch;
}
+ eb_scoop_unbound_vma_all(eb->context->vm);
+
return 0;
}
+static int eb_lock_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int err;
+
+ err = i915_gem_object_lock(eb->context->vm->root_obj, >ww);
+ if (err)
+ return err;
+
+ list_for_each_entry(vma, >non_priv_vm_bind_list,
+ non_priv_vm_bind_link) {
+ err = i915_gem_object_lock(vma->obj, >ww);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static void eb_release_persistent_vma_all(struct i915_execbuffer *eb,
+ bool final)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma, *vn;
+
+ lockdep_assert_held(>vm_bind_lock);
+
+ if (!(eb->args->flags & __EXEC3_HAS_PIN))
+ return;
+
+ assert_object_held(vm->root_obj);
+
+ list_for_each_entry(vma, >vm_bind_list, vm_bind_link)
+ __i915_vma_unpin(vma);
+
+ eb->args->flags &= ~__EXEC3_HAS_PIN;
+ if (!final)
+ return;
+
+ list_for_each_entry_safe(vma, vn, >vm_bind_list, vm_bind_link)
+ if (i915_vma_verify_bind_complete(vma))
+ list_move_tail(>vm_bind_link, >vm_bound_list);
+}
+
static void eb_release_vma_all(struct i915_execbuffer *eb, bool final)
{
+ eb_release_persistent_vma_all(eb, final);
eb_unpin_engine(eb);
}
+static int eb_reserve_fence_for_persistent_vma_all(struct i915_execbuffer *eb)
+{
+ struct i915_address_space *vm = eb->context->vm;
+ struct i915_vma *vma;
+ int ret;
+
+ ret = dma_resv_reserve_fences(vm->root_obj->base.resv, 1);
+ if (ret)
+ return ret;
+
+ list_for_each_entry(vma,
Re: [RFC/PATCH] backlight: hx8357: prepare to conversion to gpiod API
On Tue, Oct 4, 2022 at 2:54 PM Daniel Thompson wrote: > > We need to know if i.MX is shipping device trees stored in flash, > > or if they bundle it with the kernel. > > This part is frequently found in add-on boards so it's not purely an > i.MX-only question. Oh > IMHO for not-in-the-soc devices like this the presence of in-kernel DTs > isn't enough to make a decision. What is needed is a degree of > due-diligence to show that there are no obvious out-of-kernel users. OK I think this is a good case to use a special quirk in this case. I actually envisioned keeping piling them up, and that they would not be innumerable. Dmitry, could you fix this? Just patch away in gpiolib-of.c. Yours, Linus Walleij
Re: [PATCH] drm/bridge: tc358775: Do not soft reset i2c-slave controller
On Thu, 1 Sept 2022 at 15:20, Teresa Remmet wrote: > > Soft reset during tc_bridge_enable() is triggered by setting all available > reset control bits in the SYSRST register. > But as noted in the data sheet resetting the i2c-slave controller should > be only done over DSI and is only useful for chip debugging. > So do not set RSTI2CS (bit0). > > Signed-off-by: Teresa Remmet > --- > drivers/gpu/drm/bridge/tc358775.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/tc358775.c > b/drivers/gpu/drm/bridge/tc358775.c > index f1c6e62b0e1d..a5f5eae1e80f 100644 > --- a/drivers/gpu/drm/bridge/tc358775.c > +++ b/drivers/gpu/drm/bridge/tc358775.c > @@ -408,7 +408,7 @@ static void tc_bridge_enable(struct drm_bridge *bridge) > (val >> 8) & 0xFF, val & 0xFF); > > d2l_write(tc->i2c, SYSRST, SYS_RST_REG | SYS_RST_DSIRX | SYS_RST_BM | > - SYS_RST_LCD | SYS_RST_I2CM | SYS_RST_I2CS); > + SYS_RST_LCD | SYS_RST_I2CM); > usleep_range(3, 4); > > d2l_write(tc->i2c, PPI_TX_RX_TA, TTA_GET | TTA_SURE); > -- > 2.25.1 > Reviewed-by: Robert Foss Waiting a few days before applying this patch.
Re: [PATCH] drm/i915/display/lspcon: Increase LSPCON mode settle timeout
On Thu, 15 Sep 2022, Pablo Ceballos wrote: > On some devices the Parade PS175 takes more than 400ms to settle in PCON > mode. Got any bug report with more info, or any other details to back this up? This is kind of thin. What's the 800 ms based on? BR, Jani. > > Signed-off-by: Pablo Ceballos > --- > drivers/gpu/drm/i915/display/intel_lspcon.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_lspcon.c > b/drivers/gpu/drm/i915/display/intel_lspcon.c > index 15d59de8810e..b4cbade13ee5 100644 > --- a/drivers/gpu/drm/i915/display/intel_lspcon.c > +++ b/drivers/gpu/drm/i915/display/intel_lspcon.c > @@ -166,7 +166,7 @@ static enum drm_lspcon_mode lspcon_wait_mode(struct > intel_lspcon *lspcon, > drm_dbg_kms(>drm, "Waiting for LSPCON mode %s to settle\n", > lspcon_mode_name(mode)); > > - wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode, 400); > + wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode, 800); > if (current_mode != mode) > drm_err(>drm, "LSPCON mode hasn't settled\n"); -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v2 0/4] Fix HFVSDB parsing
On Fri, 16 Sep 2022, Ankit Nautiyal wrote: > Fix issues in HFVSDB parsing for DSC support. > Also minor refactoring in Logging. > > Split from original patch into a new series. > https://patchwork.freedesktop.org/patch/495193/ > > v2: Minor styling fixes. Thanks for the patches, pushed to drm-misc-next. BR, Jani. > > Ankit Nautiyal (4): > drm/edid: Fix minimum bpc supported with DSC1.2 for HDMI sink > drm/edid: Split DSC parsing into separate function > drm/edid: Refactor HFVSDB parsing for DSC1.2 > drm/edid: Avoid multiple log lines for HFVSDB parsing > > drivers/gpu/drm/drm_edid.c | 153 + > 1 file changed, 87 insertions(+), 66 deletions(-) -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 4/5] drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional bits
On 10/1/2022 12:08 PM, Marijn Suijten wrote:
According to the comment this DPU register contains the bits per pixel
as a 6.4 fractional value, conveniently matching the contents of
bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
bits. However, the downstream source this implementation was
copy-pasted from has its bpp field stored _without_ fractional part.
This makes the entire convoluted math obsolete as it is impossible to
pull those 4 fractional bits out of thin air, by somehow trying to reuse
the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
The rest of the code merely attempts to keep the integer part a multiple
of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
12; already filling up those bits anyway (but not on downstream).
Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
Signed-off-by: Marijn Suijten
Many of this bugs are because the downstream code from which this
implementation was derived wasnt the latest perhaps?
Earlier, downstream had its own DSC struct maybe leading to this
redundant math but now we have migrated over to use the upstream struct
drm_dsc_config.
That being said, this patch LGTM
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
index f2ddcfb6f7ee..3662df698dae 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -42,7 +42,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
u32 initial_lines)
{
struct dpu_hw_blk_reg_map *c = _dsc->hw;
- u32 data, lsb, bpp;
+ u32 data;
u32 slice_last_group_size;
u32 det_thresh_flatness;
bool is_cmd_mode = !(mode & DSC_MODE_VIDEO);
@@ -56,14 +56,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
data = (initial_lines << 20);
data |= ((slice_last_group_size - 1) << 18);
/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
- data |= dsc->bits_per_pixel << 12;
- lsb = dsc->bits_per_pixel % 4;
- bpp = dsc->bits_per_pixel / 4;
- bpp *= 4;
- bpp <<= 4;
- bpp |= lsb;
-
- data |= bpp << 8;
+ data |= (dsc->bits_per_pixel << 8);
data |= (dsc->block_pred_enable << 7);
data |= (dsc->line_buf_depth << 3);
data |= (dsc->simple_422 << 2);
Re: [PATCH 1/4] dt-bindings: display: Add bindings for JDI LPM102A188A
On Tue, Oct 04, 2022 at 01:05:04PM +0200, Krzysztof Kozlowski wrote: > On 03/10/2022 19:06, Diogo Ivo wrote: > > On Fri, Sep 30, 2022 at 12:49:31PM +0200, Krzysztof Kozlowski wrote: > >> Isn't touchscreen a separate (input) device? > > > > Hello, thank you for the feedback. > > > > According to the downstream kernel's log, it seems like the panel and > > the touchscreen controller are considered to be embedded in the same unit > > (for example in [1]), > > Downstream kernel is not a proof of proper description of hardware. If > downstream says orange is an apple, does it mean orange is really an > apple? No... Downstream creates a lot of junk, hacks and workarounds. After some searching (which I should have done sooner, so apologies) I came across a teardown of the Pixel C ([1], for completeness), which incorporates this panel. Indeed a separate touch controller was found, so it seems the downstream kernel threw me off as per your warning. [1]: https://www.ifixit.com/Teardown/Google+Pixel+C+Teardown/62277 (Step 4) > > with the touch input being transmitted via HID-over-I2C, > > and since I did not find any reset gpio handling in that driver I opted to > > include this reset here, unless there is a better way of going about this. > > Instead it should be in touch screen device. Noted, I will remove it from the binding in the next version. > Where is the DTS of that device? The relevant part of the DTS can be found here: https://android.googlesource.com/kernel/tegra/+/refs/heads/android-tegra-dragon-3.18-oreo/arch/arm64/boot/dts/tegra/tegra210-smaug.dtsi Best regards, Diogo
Re: [PATCH -next] drm/msm: Remove unused variables 'top'
On 10/9/2022 7:40 PM, Chen Zhongjin wrote:
'commit 1e5df24b996c ("drm/msm/dpu: drop length from struct
dpu_hw_blk_reg_map")'
'commit 9403f9a42c88 ("drm/msm/dpu: merge base_off with blk_off in struct
dpu_hw_blk_reg_map")'
These commits had merged hw.blk_off and hw.blk_off to mdp.
So we don't need to get dpu_hw_mdp in dpu_kms_mdp_snapshot() now.
Since there is no code using 'top' in this function. Remove it.
Signed-off-by: Chen Zhongjin
This has already been fixed with:
https://gitlab.freedesktop.org/drm/msm/-/commit/4bca876458caf7c105ab2ae9d80ff2cc9c60388d
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 008e1420e6e5..79e81f1443be 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -902,13 +902,9 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state
*disp_state, struct msm_k
int i;
struct dpu_kms *dpu_kms;
const struct dpu_mdss_cfg *cat;
- struct dpu_hw_mdp *top;
dpu_kms = to_dpu_kms(kms);
-
cat = dpu_kms->catalog;
- top = dpu_kms->hw_mdp;
-
pm_runtime_get_sync(_kms->pdev->dev);
/* dump CTL sub-blocks HW regs info */
Re: [PATCH] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge
On Mon, 29 Aug 2022 at 20:23, Abhinav Kumar wrote:
>
> adv7533 bridge tries to dynamically switch lanes based on the
> mode by detaching and attaching the mipi dsi device.
>
> This approach is incorrect because this method of dynamic switch of
> detaching and attaching the mipi dsi device also results in removing
> and adding the component which is not necessary.
>
> This approach is also prone to deadlocks. So for example, on the
> db410c whenever this path is executed with lockdep enabled,
> this results in a deadlock due to below ordering of locks.
>
> -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
> lock_acquire+0x6c/0x90
> drm_modeset_acquire_init+0xf4/0x150
> drmm_mode_config_init+0x220/0x770
> msm_drm_bind+0x13c/0x654
> try_to_bring_up_aggregate_device+0x164/0x1d0
> __component_add+0xa8/0x174
> component_add+0x18/0x2c
> dsi_dev_attach+0x24/0x30
> dsi_host_attach+0x98/0x14c
> devm_mipi_dsi_attach+0x38/0xb0
> adv7533_attach_dsi+0x8c/0x110
> adv7511_probe+0x5a0/0x930
> i2c_device_probe+0x30c/0x350
> really_probe.part.0+0x9c/0x2b0
> __driver_probe_device+0x98/0x144
> driver_probe_device+0xac/0x14c
> __device_attach_driver+0xbc/0x124
> bus_for_each_drv+0x78/0xd0
> __device_attach+0xa8/0x1c0
> device_initial_probe+0x18/0x24
> bus_probe_device+0xa0/0xac
> deferred_probe_work_func+0x90/0xd0
> process_one_work+0x28c/0x6b0
> worker_thread+0x240/0x444
> kthread+0x110/0x114
> ret_from_fork+0x10/0x20
>
> -> #0 (component_mutex){+.+.}-{3:3}:
> __lock_acquire+0x1280/0x20ac
> lock_acquire.part.0+0xe0/0x230
> lock_acquire+0x6c/0x90
> __mutex_lock+0x84/0x400
> mutex_lock_nested+0x3c/0x70
> component_del+0x34/0x170
> dsi_dev_detach+0x24/0x30
> dsi_host_detach+0x20/0x64
> mipi_dsi_detach+0x2c/0x40
> adv7533_mode_set+0x64/0x90
> adv7511_bridge_mode_set+0x210/0x214
> drm_bridge_chain_mode_set+0x5c/0x84
> crtc_set_mode+0x18c/0x1dc
> drm_atomic_helper_commit_modeset_disables+0x40/0x50
> msm_atomic_commit_tail+0x1d0/0x6e0
> commit_tail+0xa4/0x180
> drm_atomic_helper_commit+0x178/0x3b0
> drm_atomic_commit+0xa4/0xe0
> drm_client_modeset_commit_atomic+0x228/0x284
> drm_client_modeset_commit_locked+0x64/0x1d0
> drm_client_modeset_commit+0x34/0x60
> drm_fb_helper_lastclose+0x74/0xcc
> drm_lastclose+0x3c/0x80
> drm_release+0xfc/0x114
> __fput+0x70/0x224
> fput+0x14/0x20
> task_work_run+0x88/0x1a0
> do_exit+0x350/0xa50
> do_group_exit+0x38/0xa4
> __wake_up_parent+0x0/0x34
> invoke_syscall+0x48/0x114
> el0_svc_common.constprop.0+0x60/0x11c
> do_el0_svc+0x30/0xc0
> el0_svc+0x58/0x100
> el0t_64_sync_handler+0x1b0/0x1bc
> el0t_64_sync+0x18c/0x190
>
> Due to above reasons, remove the dynamic lane switching
> code from adv7533 bridge chip and filter out the modes
> which would need different number of lanes as compared
> to the initialization time using the mode_valid callback.
>
> This can be potentially re-introduced by using the pre_enable()
> callback but this needs to be evaluated first whether such an
> approach will work so this will be done with a separate change.
>
> changes since RFC:
> - Fix commit text and add TODO comment
>
> Fixes: 62b2f026cd8e ("drm/bridge: adv7533: Change number of DSI lanes
> dynamically")
> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/16
> Suggested-by: Dmitry Baryshkov
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511.h | 3 ++-
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 18 ++
> drivers/gpu/drm/bridge/adv7511/adv7533.c | 25 +
> 3 files changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index 9e3bb8a8ee40..0a7cec80b75d 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -417,7 +417,8 @@ static inline int adv7511_cec_init(struct device *dev,
> struct adv7511 *adv7511)
>
> void adv7533_dsi_power_on(struct adv7511 *adv);
> void adv7533_dsi_power_off(struct adv7511 *adv);
> -void adv7533_mode_set(struct adv7511 *adv, const struct drm_display_mode
> *mode);
> +enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv,
> + const struct drm_display_mode *mode);
> int adv7533_patch_registers(struct adv7511 *adv);
> int adv7533_patch_cec_registers(struct adv7511 *adv);
> int adv7533_attach_dsi(struct adv7511 *adv);
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index
Re: [Intel-gfx] [PATCH v2] drm/i915/guc: Fix revocation of non-persistent contexts
On 10/4/2022 4:14 AM, Tvrtko Ursulin wrote:
On 03/10/2022 13:16, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Patch which added graceful exit for non-persistent contexts missed the
fact it is not enough to set the exiting flag on a context and let the
backend handle it from there.
GuC backend cannot handle it because it runs independently in the
firmware and driver might not see the requests ever again. Patch also
missed the fact some usages of intel_context_is_banned in the GuC
backend
needed replacing with newly introduced intel_context_is_schedulable.
Fix the first issue by calling into backend revoke when we know this is
the last chance to do it. Fix the second issue by replacing
intel_context_is_banned with intel_context_is_schedulable, which should
always be safe since latter is a superset of the former.
v2:
* Just call ce->ops->revoke unconditionally. (Andrzej)
CI is happy - could I get some acks for the GuC backend changes please?
I think we still need to have a longer conversation on the revoking
times, but in the meantime this fixes the immediate concerns, so:
Acked-by: Daniele Ceraolo Spurio
Daniele
Regards,
Tvrtko
Signed-off-by: Tvrtko Ursulin
Fixes: 45c64ecf97ee ("drm/i915: Improve user experience and driver
robustness under SIGINT or similar")
Cc: Andrzej Hajda
Cc: John Harrison
Cc: Daniele Ceraolo Spurio
Cc: # v6.0+
Reviewed-by: Andrzej Hajda
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 8 +-
drivers/gpu/drm/i915/gt/intel_context.c | 5 ++--
drivers/gpu/drm/i915/gt/intel_context.h | 3 +--
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 26 +--
4 files changed, 17 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 0bcde53c50c6..1e29b1e6d186 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1387,14 +1387,8 @@ kill_engines(struct i915_gem_engines *engines,
bool exit, bool persistent)
*/
for_each_gem_engine(ce, engines, it) {
struct intel_engine_cs *engine;
- bool skip = false;
- if (exit)
- skip = intel_context_set_exiting(ce);
- else if (!persistent)
- skip = intel_context_exit_nonpersistent(ce, NULL);
-
- if (skip)
+ if ((exit || !persistent) && intel_context_revoke(ce))
continue; /* Already marked. */
/*
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index 654a092ed3d6..e94365b08f1e 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -614,13 +614,12 @@ bool intel_context_ban(struct intel_context
*ce, struct i915_request *rq)
return ret;
}
-bool intel_context_exit_nonpersistent(struct intel_context *ce,
- struct i915_request *rq)
+bool intel_context_revoke(struct intel_context *ce)
{
bool ret = intel_context_set_exiting(ce);
if (ce->ops->revoke)
- ce->ops->revoke(ce, rq, ce->engine->props.preempt_timeout_ms);
+ ce->ops->revoke(ce, NULL,
ce->engine->props.preempt_timeout_ms);
return ret;
}
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
b/drivers/gpu/drm/i915/gt/intel_context.h
index 8e2d70630c49..be09fb2e883a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -329,8 +329,7 @@ static inline bool
intel_context_set_exiting(struct intel_context *ce)
return test_and_set_bit(CONTEXT_EXITING, >flags);
}
-bool intel_context_exit_nonpersistent(struct intel_context *ce,
- struct i915_request *rq);
+bool intel_context_revoke(struct intel_context *ce);
static inline bool
intel_context_force_single_submission(const struct intel_context *ce)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 0ef295a94060..88a4476b8e92 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -685,7 +685,7 @@ static int __guc_add_request(struct intel_guc
*guc, struct i915_request *rq)
* Corner case where requests were sitting in the priority list
or a
* request resubmitted after the context was banned.
*/
- if (unlikely(intel_context_is_banned(ce))) {
+ if (unlikely(!intel_context_is_schedulable(ce))) {
i915_request_put(i915_request_mark_eio(rq));
intel_engine_signal_breadcrumbs(ce->engine);
return 0;
@@ -871,15 +871,15 @@ static int guc_wq_item_append(struct intel_guc
*guc,
struct i915_request *rq)
{
struct intel_context *ce = request_to_scheduling_context(rq);
- int ret = 0;
+ int ret;
- if (likely(!intel_context_is_banned(ce))) {
- ret = __guc_wq_item_append(rq);
+ if
Re: [PATCH v1 17/17] drm/mediatek: Add mt8195-dpi support to drm_drv
On 04/10/2022 13:55, Guillaume Ranquet wrote:
>> No. You said what the code is doing. I think I understand this. You
>> still do not need more compatibles. Your sentence did not clarify it
>> because it did not answer at all to question "why". Why do you need it?
>>
>> Sorry, the change looks not correct.
>>
>> Best regards,
>> Krzysztof
>>
>
> I need a new compatible to adress the specifics of mt8195 in the mtk_dpi
> driver,
> the change is in this series with:
> [PATCH v1 16/17] drm/mediatek: dpi: Add mt8195 hdmi to DPI driver [1]
But you do not have specifics of mt8195. I wrote it in the beginning.
>
> I then need to add that compatible to the "list" here in mtk_drm_drv.
No, you do not... I checked the driver and there is no single need... or
convince me you need.
> I don't see a way around this unless I rewrite the way mtk_drm_drv works?
Why rewrite? You have all compatibles in place.
>
> Maybe if I declare a new compatible that is generic to all mediatek
> dpi variants?
You were asked to use fallback. Don't create some fake fallbacks. Use
existing ones.
> and have all the dts specify the node with both the generic dpi and
> the specific compatible?
>
> dpi@xxx {
> compatible = "mediatek,dpi", "mediatek,mt8195-dpi";
I don't know what's this but certainly looks odd. Some wild-card
compatible in front (not fallback) and none are documented.
> ...
> }
>
> Then I can "collapse" all the dpi related nodes in mtk_drm_drv under
> "mediatek,dpi" ?
>
> I guess would have to do the change for all other components that are needed
> in
> mtk_drm_drv (mmsys, aal, ccor, color, dither, dsc, gamma, mutex...).
>
> That's the only trivial way I can think of implementing this with the
> current status
> of the mtk_drm stack.
>
> Do you have any other ideas in mind?
Use fallback of compatible device. That's the common pattern.
Everywhere, Mediatek as well.
Best regards,
Krzysztof
Re: [PATCH] dt-bindings: display: panel: use spi-peripheral-props.yaml
On 04/10/2022 15:17, Laurent Pinchart wrote:
> Hi Krzysztof,
>
> On Tue, Oct 04, 2022 at 03:10:29PM +0200, Krzysztof Kozlowski wrote:
>> On 04/10/2022 15:03, Laurent Pinchart wrote:
>>> On Tue, Oct 04, 2022 at 02:09:07PM +0200, Krzysztof Kozlowski wrote:
For devices connectable by SPI bus (e.g. already using
"spi-max-frequency" property), reference the "spi-peripheral-props.yaml"
schema to allow using all SPI device properties, even these which device
bindings author did not tried yet.
>>>
>>> Isn't this done implicitly by spi-controller.yaml ? SPI devices that are
>>> children of an SPI controller should match the patternProperties
>>> "^.*@[0-9a-f]+$" in that file, which has a $ref: spi-peripheral-props.yaml.
>>> Is there something I'm missing ?
>>
>> You are correct about one side of this - SPI controller bindings.
>> However these schemas here have clear: additional/unevaluatedProperties:
>> false, thus when they find DTS like:
>> panel@xxx {
>> compatible = "one of these spi panels";
>> ...
>> spi-cs-high;
>> spi-rx-delay-us = <50>;
>> ... and some more from specific controllers
>> }
>>
>> you will get errors, because the panel schema does not allow them.
>>
>> The bindings were done (some time ago) in such way, that they require
>> that both SPI controller and SPI device reference spi-props.
>
> You're absolutely right that additionalProperties needs to be replaced
> by unevaluatedProperties. Can the additions of $ref be dropped, or is
> that needed too ?
I just wrote above - you need to reference the spi-props. Otherwise all
the SPI-related properties will be unknown/unevaluated.
Best regards,
Krzysztof
Re: [PATCH] drm/amd/display: Remove unused struct i2c_id_config_access
On 2022-09-27 09:39, Yuan Can wrote:
After commit 5a8132b9f606("drm/amd/display: remove dead dc vbios code"), no one
use struct i2c_id_config_access, so remove it.
Signed-off-by: Yuan Can
---
drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c
b/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c
index 5d70f9901d13..d380cf98b844 100644
--- a/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c
+++ b/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c
@@ -50,13 +50,6 @@
#define LAST_RECORD_TYPE 0xff
#define SMU9_SYSPLL0_ID 0
-struct i2c_id_config_access {
- uint8_t bfI2C_LineMux:4;
- uint8_t bfHW_EngineID:3;
- uint8_t bfHW_Capable:1;
- uint8_t ucAccess;
-};
-
static enum bp_result get_gpio_i2c_info(struct bios_parser *bp,
struct atom_i2c_record *record,
struct graphics_object_i2c_info *info);
Reviewed-by: Rodrigo Siqueira
and applied to amd-staging-drm-next.
Thanks
Siqueira
Re: [PATCH -next] drm/amd/display: Removed unused variable 'sdp_stream_enable'
On 2022-09-30 02:38, Dong Chenchen wrote:
Kernel test robot throws below warning ->
drivers/gpu/drm/amd/amdgpu/../display/dc/dcn31/dcn31_hpo_dp_stream_encoder.c:
In function 'dcn31_hpo_dp_stream_enc_update_dp_info_packets':
drivers/gpu/drm/amd/amdgpu/../display/dc/dcn31/dcn31_hpo_dp_stream_encoder.c:439:14:
warning: variable 'sdp_stream_enable' set but not used
[-Wunused-but-set-variable]
439 | bool sdp_stream_enable = false;
Removed unused variable 'sdp_stream_enable'.
Reported-by: kernel test robot
Signed-off-by: Dong Chenchen
---
.../dc/dcn31/dcn31_hpo_dp_stream_encoder.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hpo_dp_stream_encoder.c
b/drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hpo_dp_stream_encoder.c
index 23621ff08c90..7daafbab98da 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hpo_dp_stream_encoder.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn31/dcn31_hpo_dp_stream_encoder.c
@@ -436,32 +436,28 @@ static void
dcn31_hpo_dp_stream_enc_update_dp_info_packets(
{
struct dcn31_hpo_dp_stream_encoder *enc3 =
DCN3_1_HPO_DP_STREAM_ENC_FROM_HPO_STREAM_ENC(enc);
uint32_t dmdata_packet_enabled = 0;
- bool sdp_stream_enable = false;
- if (info_frame->vsc.valid) {
+ if (info_frame->vsc.valid)
enc->vpg->funcs->update_generic_info_packet(
enc->vpg,
0, /* packetIndex */
_frame->vsc,
true);
- sdp_stream_enable = true;
- }
- if (info_frame->spd.valid) {
+
+ if (info_frame->spd.valid)
enc->vpg->funcs->update_generic_info_packet(
enc->vpg,
2, /* packetIndex */
_frame->spd,
true);
- sdp_stream_enable = true;
- }
- if (info_frame->hdrsmd.valid) {
+
+ if (info_frame->hdrsmd.valid)
enc->vpg->funcs->update_generic_info_packet(
enc->vpg,
3, /* packetIndex */
_frame->hdrsmd,
true);
- sdp_stream_enable = true;
- }
+
/* enable/disable transmission of packet(s).
* If enabled, packet transmission begins on the next frame
*/
Thanks a lot for your patch,
Reviewed-by: Rodrigo Siqueira
and applied to amd-staging-drm-next.
Thanks
Siqueira
Re: [PATCH 3/5] drm/msm/dsi: Account for DSC's bits_per_pixel having 4 fractional bits
On Sat, 1 Oct 2022 at 22:08, Marijn Suijten
wrote:
>
> drm_dsc_config's bits_per_pixel field holds a fractional value with 4
> bits, which all panel drivers should adhere to for
> drm_dsc_pps_payload_pack() to generate a valid payload. All code in the
> DSI driver here seems to assume that this field doesn't contain any
> fractional bits, hence resulting in the wrong values being computed.
> Since none of the calculations leave any room for fractional bits or
> seem to indicate any possible area of support, disallow such values
> altogether.
>
> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> Signed-off-by: Marijn Suijten
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 34 +++---
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index cb6f2fa11f58..42a5c9776f52 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -847,6 +847,11 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> u32 pkt_per_line;
> u32 bytes_in_slice;
> u32 eol_byte_num;
> + int bpp = dsc->bits_per_pixel >> 4;
> +
> + if (dsc->bits_per_pixel & 0xf)
> + /* dsi_populate_dsc_params() already caught this case */
> + pr_err("DSI does not support fractional bits_per_pixel\n");
>
> /* first calculate dsc parameters and then program
> * compress mode registers
> @@ -860,7 +865,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
> *msm_host, bool is_cmd_mod
> if (slice_per_intf > dsc->slice_count)
> dsc->slice_count = 1;
>
> - bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel,
> 8);
> + bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * bpp, 8);
bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8 * 16); ?
>
> dsc->slice_chunk_size = bytes_in_slice;
>
> @@ -913,6 +918,7 @@ static void dsi_timing_setup(struct msm_dsi_host
> *msm_host, bool is_bonded_dsi)
> u32 va_end = va_start + mode->vdisplay;
> u32 hdisplay = mode->hdisplay;
> u32 wc;
> + int ret;
>
> DBG("");
>
> @@ -948,7 +954,9 @@ static void dsi_timing_setup(struct msm_dsi_host
> *msm_host, bool is_bonded_dsi)
> /* we do the calculations for dsc parameters here so that
> * panel can use these parameters
> */
> - dsi_populate_dsc_params(dsc);
> + ret = dsi_populate_dsc_params(dsc);
> + if (ret)
> + return;
>
> /* Divide the display by 3 but keep back/font porch and
> * pulse width same
> @@ -1229,6 +1237,10 @@ static int dsi_cmd_dma_add(struct msm_dsi_host
> *msm_host,
> if (packet.size < len)
> memset(data + packet.size, 0xff, len - packet.size);
>
> + if (msg->type == MIPI_DSI_PICTURE_PARAMETER_SET)
> + print_hex_dump(KERN_DEBUG, "ALL:", DUMP_PREFIX_NONE,
> + 16, 1, data, len, false);
> +
> if (cfg_hnd->ops->tx_buf_put)
> cfg_hnd->ops->tx_buf_put(msm_host);
>
> @@ -1786,6 +1798,12 @@ static int dsi_populate_dsc_params(struct
> drm_dsc_config *dsc)
> int data;
> int final_value, final_scale;
> int i;
> + int bpp = dsc->bits_per_pixel >> 4;
> +
> + if (dsc->bits_per_pixel & 0xf) {
> + pr_err("DSI does not support fractional bits_per_pixel\n");
> + return -EINVAL;
> + }
>
> dsc->rc_model_size = 8192;
> dsc->first_line_bpg_offset = 12;
> @@ -1807,7 +1825,7 @@ static int dsi_populate_dsc_params(struct
> drm_dsc_config *dsc)
> }
>
> dsc->initial_offset = 6144; /* Not bpp 12 */
> - if (dsc->bits_per_pixel != 8)
> + if (bpp != 8)
> dsc->initial_offset = 2048; /* bpp = 12 */
>
> mux_words_size = 48;/* bpc == 8/10 */
> @@ -1830,16 +1848,16 @@ static int dsi_populate_dsc_params(struct
> drm_dsc_config *dsc)
> * params are calculated
> */
> groups_per_line = DIV_ROUND_UP(dsc->slice_width, 3);
> - dsc->slice_chunk_size = dsc->slice_width * dsc->bits_per_pixel / 8;
> - if ((dsc->slice_width * dsc->bits_per_pixel) % 8)
> + dsc->slice_chunk_size = dsc->slice_width * bpp / 8;
> + if ((dsc->slice_width * bpp) % 8)
One can use fixed point math here too:
dsc->slice_chunk_size = (dsc->slice_width * dsc->bits_per_pixel + 8 *
16 - 1)/ (8 * 16);
> dsc->slice_chunk_size++;
>
> /* rbs-min */
> min_rate_buffer_size = dsc->rc_model_size - dsc->initial_offset +
> - dsc->initial_xmit_delay * dsc->bits_per_pixel
> +
> + dsc->initial_xmit_delay * bpp +
>
Re: [PATCH 2/5] drm/msm/dsi: Remove repeated calculation of slice_per_intf
On 10/1/2022 12:08 PM, Marijn Suijten wrote:
slice_per_intf is already computed for intf_width, which holds the same
value as hdisplay.
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Marijn Suijten
LGTM,
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e05bae647431..cb6f2fa11f58 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -842,7 +842,7 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host,
bool enable,
static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool
is_cmd_mode, u32 hdisplay)
{
struct drm_dsc_config *dsc = msm_host->dsc;
- u32 reg, intf_width, reg_ctrl, reg_ctrl2;
+ u32 reg, reg_ctrl, reg_ctrl2;
u32 slice_per_intf, total_bytes_per_intf;
u32 pkt_per_line;
u32 bytes_in_slice;
@@ -851,8 +851,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
/* first calculate dsc parameters and then program
* compress mode registers
*/
- intf_width = hdisplay;
- slice_per_intf = DIV_ROUND_UP(intf_width, dsc->slice_width);
+ slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
/* If slice_per_pkt is greater than slice_per_intf
* then default to 1. This can happen during partial
@@ -861,7 +860,6 @@ static void dsi_update_dsc_timing(struct msm_dsi_host
*msm_host, bool is_cmd_mod
if (slice_per_intf > dsc->slice_count)
dsc->slice_count = 1;
- slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width);
bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel,
8);
dsc->slice_chunk_size = bytes_in_slice;
Re: [PATCH 5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields
On Sat, 1 Oct 2022 at 23:23, Marijn Suijten
wrote:
>
> On 2022-10-01 21:08:07, Marijn Suijten wrote:
> > msm's dsi_host specifies negative BPG offsets which fill the full 8 bits
> > of a char thanks to two's complement: this however results in those bits
> > bleeding into the next parameter when the field is only expected to
> > contain 6-bit wide values.
> > As a consequence random slices appear corrupted on-screen (tested on a
> > Sony Tama Akatsuki device with sdm845).
> >
> > Use AND operators to limit all values that constitute the RC Range
> > parameter fields to their expected size.
> >
> > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> > Signed-off-by: Marijn Suijten
> > ---
> > drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c
> > b/drivers/gpu/drm/display/drm_dsc_helper.c
> > index c869c6e51e2b..2e7ef242685d 100644
> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> > @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct
> > drm_dsc_picture_parameter_set *pps_payload,
> >*/
> > for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> > pps_payload->rc_range_parameters[i] =
> > - cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp
> > <<
> > +
> > cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
> >DSC_PPS_RC_RANGE_MINQP_SHIFT) |
> > - (dsc_cfg->rc_range_params[i].range_max_qp
> > <<
> > +
> > ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
> >DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
> > -
> > (dsc_cfg->rc_range_params[i].range_bpg_offset));
> > +
> > (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
>
> Pre-empting the reviews: I was contemplating whether to use FIELD_PREP
> here, given that it's not yet used anywhere else in this file. For that
> I'd remove the existing _SHIFT definitions and replace them with:
>
> #define DSC_PPS_RC_RANGE_MINQP_MASK GENMASK(15, 11)
> #define DSC_PPS_RC_RANGE_MAXQP_MASK GENMASK(10, 6)
> #define DSC_PPS_RC_RANGE_BPG_OFFSET_MASKGENMASK(5, 0)
>
> And turn this section of code into:
>
> cpu_to_be16(FIELD_PREP(DSC_PPS_RC_RANGE_MINQP_MASK,
>dsc_cfg->rc_range_params[i].range_min_qp) |
> FIELD_PREP(DSC_PPS_RC_RANGE_MAXQP_MASK,
>dsc_cfg->rc_range_params[i].range_max_qp) |
> FIELD_PREP(DSC_PPS_RC_RANGE_BPG_OFFSET_MASK,
>dsc_cfg->rc_range_params[i].range_bpg_offset));
>
> Is that okay/recommended?
This is definitely easier to review. However if you do not want to use
FIELD_PREP, it would be better to split this into a series of `data |=
something` assignments terminated with the rc_range_parameters[i]
assignment.
>
> - Marijn
>
> > }
> >
> > /* PPS 88 */
> > --
> > 2.37.3
> >
--
With best wishes
Dmitry
Re: [PATCH 4/5] drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional bits
On Sat, 1 Oct 2022 at 22:08, Marijn Suijten
wrote:
>
> According to the comment this DPU register contains the bits per pixel
> as a 6.4 fractional value, conveniently matching the contents of
> bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
> bits. However, the downstream source this implementation was
> copy-pasted from has its bpp field stored _without_ fractional part.
>
> This makes the entire convoluted math obsolete as it is impossible to
> pull those 4 fractional bits out of thin air, by somehow trying to reuse
> the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
>
> The rest of the code merely attempts to keep the integer part a multiple
> of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
> 12; already filling up those bits anyway (but not on downstream).
>
> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> Signed-off-by: Marijn Suijten
Reviewed-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 11 ++-
> 1 file changed, 2 insertions(+), 9 deletions(-)
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 1/5] drm/msm/dsi: Remove useless math in DSC calculation
On 10/1/2022 12:08 PM, Marijn Suijten wrote:
Multiplying a value by 2 and adding 1 to it always results in a value
that is uneven, and that 1 gets truncated immediately when performing
integer division by 2 again. There is no "rounding" possible here.
Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 8e4bc586c262..e05bae647431 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1864,12 +1864,7 @@ static int dsi_populate_dsc_params(struct drm_dsc_config
*dsc)
data = 2048 * (dsc->rc_model_size - dsc->initial_offset +
num_extra_mux_bits);
dsc->slice_bpg_offset = DIV_ROUND_UP(data, groups_total);
- /* bpp * 16 + 0.5 */
- data = dsc->bits_per_pixel * 16;
- data *= 2;
- data++;
- data /= 2;
- target_bpp_x16 = data;
+ target_bpp_x16 = dsc->bits_per_pixel * 16;
Since this patch is titled, "remove useless math", even the
target_bpp_x16 math looks redundant to me,
first we do
target_bpp_x16 = dsc->bits_per_pixel * 16;
then in the next line we do
data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
the *16 and /16 will cancel out here.
Instead we can just do
data = (dsc->initial_xmit_delay * dsc->drm->bits_per_pixel) ?
data = (dsc->initial_xmit_delay * target_bpp_x16) / 16;
final_value = dsc->rc_model_size - data + num_extra_mux_bits;
Re: [v6] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280
On Sun, 2 Oct 2022 at 06:15, Kalyan Thota wrote:
>
> Flush mechanism for DSPP blocks has changed in sc7280 family, it
> allows individual sub blocks to be flushed in coordination with
> master flush control.
>
> Representation: master_flush && (PCC_flush | IGC_flush .. etc )
>
> This change adds necessary support for the above design.
>
> Changes in v1:
> - Few nits (Doug, Dmitry)
> - Restrict sub-block flush programming to dpu_hw_ctl file (Dmitry)
>
> Changes in v2:
> - Move the address offset to flush macro (Dmitry)
> - Seperate ops for the sub block flush (Dmitry)
>
> Changes in v3:
> - Reuse the DPU_DSPP_xx enum instead of a new one (Dmitry)
>
> Changes in v4:
> - Use shorter version for unsigned int (Stephen)
>
> Changes in v5:
> - Spurious patch please ignore.
>
> Changes in v6:
> - Add SOB tag (Doug, Dmitry)
>
> Signed-off-by: Kalyan Thota
> Reviewed-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 5 +++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 4 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 35
> --
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 10 ++--
> 5 files changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 601d687..4170fbe 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -766,7 +766,7 @@ static void _dpu_crtc_setup_cp_blocks(struct drm_crtc
> *crtc)
>
> /* stage config flush mask */
> ctl->ops.update_pending_flush_dspp(ctl,
> - mixer[i].hw_dspp->idx);
> + mixer[i].hw_dspp->idx, DPU_DSPP_PCC);
> }
> }
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index 27f029f..0eecb2f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -65,7 +65,10 @@
> (PINGPONG_SDM845_MASK | BIT(DPU_PINGPONG_TE2))
>
> #define CTL_SC7280_MASK \
> - (BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE) |
> BIT(DPU_CTL_VM_CFG))
> + (BIT(DPU_CTL_ACTIVE_CFG) | \
> +BIT(DPU_CTL_FETCH_ACTIVE) | \
> +BIT(DPU_CTL_VM_CFG) | \
> +BIT(DPU_CTL_DSPP_SUB_BLOCK_FLUSH))
>
> #define MERGE_3D_SM8150_MASK (0)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index 38aa38a..8148e91 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -161,10 +161,12 @@ enum {
> * DSPP sub-blocks
> * @DPU_DSPP_PCC Panel color correction block
> * @DPU_DSPP_GC Gamma correction block
> + * @DPU_DSPP_IGC Inverse Gamma correction block
> */
> enum {
> DPU_DSPP_PCC = 0x1,
> DPU_DSPP_GC,
> + DPU_DSPP_IGC,
> DPU_DSPP_MAX
> };
>
> @@ -191,6 +193,7 @@ enum {
> * @DPU_CTL_SPLIT_DISPLAY: CTL supports video mode split display
> * @DPU_CTL_FETCH_ACTIVE: Active CTL for fetch HW (SSPPs)
> * @DPU_CTL_VM_CFG:CTL config to support multiple VMs
> + * @DPU_CTL_DSPP_BLOCK_FLUSH: CTL config to support dspp sub-block flush
> * @DPU_CTL_MAX
> */
> enum {
> @@ -198,6 +201,7 @@ enum {
> DPU_CTL_ACTIVE_CFG,
> DPU_CTL_FETCH_ACTIVE,
> DPU_CTL_VM_CFG,
> + DPU_CTL_DSPP_SUB_BLOCK_FLUSH,
> DPU_CTL_MAX
> };
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> index a35ecb6..f26f484 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> @@ -33,6 +33,7 @@
> #define CTL_INTF_FLUSH0x110
> #define CTL_INTF_MASTER 0x134
> #define CTL_FETCH_PIPE_ACTIVE 0x0FC
> +#define CTL_DSPP_n_FLUSH(n) ((0x13C) + ((n - 1) * 4))
>
> #define CTL_MIXER_BORDER_OUTBIT(24)
> #define CTL_FLUSH_MASK_CTL BIT(17)
> @@ -287,8 +288,9 @@ static void
> dpu_hw_ctl_update_pending_flush_merge_3d_v1(struct dpu_hw_ctl *ctx,
> }
>
> static void dpu_hw_ctl_update_pending_flush_dspp(struct dpu_hw_ctl *ctx,
> - enum dpu_dspp dspp)
> + enum dpu_dspp dspp, u32 dspp_sub_blk)
> {
> +
> switch (dspp) {
> case DSPP_0:
> ctx->pending_flush_mask |= BIT(13);
> @@ -307,6 +309,31 @@ static void dpu_hw_ctl_update_pending_flush_dspp(struct
> dpu_hw_ctl *ctx,
> }
> }
>
> +static void dpu_hw_ctl_update_pending_flush_dspp_subblocks(
> + struct dpu_hw_ctl *ctx, enum dpu_dspp dspp, u32 dspp_sub_blk)
> +{
> + u32 flushbits = 0, active;
> +
> + switch (dspp_sub_blk) {
> + case DPU_DSPP_IGC:
> + flushbits =
Re: [PATCH v3 2/2] drm/msm/dsi: Add phy configuration for QCM2290
On Sat, 1 Oct 2022 at 19:00, Marijn Suijten
wrote:
>
> On 2022-09-24 15:19:00, Dmitry Baryshkov wrote:
> > From: Loic Poulain
> >
> > The QCM2290 SoC a the 14nm (V2.0) single DSI phy. The platform is not
> > fully compatible with the standard 14nm PHY, so it requires a separate
> > compatible and config entry.
> >
> > Signed-off-by: Loic Poulain
> > [DB: rebased and updated commit msg]
> > Signed-off-by: Dmitry Baryshkov
> > ---
> > drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 ++
> > drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 +
> > drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 17 +
> > 3 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > index 7fc0975cb869..ee6051367679 100644
> > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
> > @@ -549,6 +549,8 @@ static const struct of_device_id dsi_phy_dt_match[] = {
> > #ifdef CONFIG_DRM_MSM_DSI_14NM_PHY
> > { .compatible = "qcom,dsi-phy-14nm",
> > .data = _phy_14nm_cfgs },
> > + { .compatible = "qcom,dsi-phy-14nm-2290",
> > + .data = _phy_14nm_2290_cfgs },
> > { .compatible = "qcom,dsi-phy-14nm-660",
> > .data = _phy_14nm_660_cfgs },
> > { .compatible = "qcom,dsi-phy-14nm-8953",
> > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > index 60a99c6525b2..1096afedd616 100644
> > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
> > @@ -50,6 +50,7 @@ extern const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs;
> > extern const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs;
> > extern const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs;
> > extern const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs;
> > +extern const struct msm_dsi_phy_cfg dsi_phy_14nm_2290_cfgs;
>
> following alphabetical sorting (same as the other locations in this
> series), this should be above 660?
Ack
>
> > extern const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs;
> > extern const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs;
> > extern const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs;
> > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> > index 0f8f4ca46429..9f488adea7f5 100644
> > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c
> > @@ -1081,3 +1081,20 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs
> > = {
> > .io_start = { 0x1a94400, 0x1a96400 },
> > .num_dsi_phy = 2,
> > };
> > +
> > +const struct msm_dsi_phy_cfg dsi_phy_14nm_2290_cfgs = {
> > + .has_phy_lane = true,
> > + .regulator_data = dsi_phy_14nm_17mA_regulators,
> > + .num_regulators = ARRAY_SIZE(dsi_phy_14nm_17mA_regulators),
> > + .ops = {
> > + .enable = dsi_14nm_phy_enable,
> > + .disable = dsi_14nm_phy_disable,
> > + .pll_init = dsi_pll_14nm_init,
> > + .save_pll_state = dsi_14nm_pll_save_state,
> > + .restore_pll_state = dsi_14nm_pll_restore_state,
> > + },
> > + .min_pll_rate = VCO_MIN_RATE,
> > + .max_pll_rate = VCO_MAX_RATE,
> > + .io_start = { 0x5e94400 },
>
> For sm6125 we also need this exact io_start (and a single PHY), do you
> think it makes sense to add a compatible that reuses the same struct (I
> can do that in a folloup patch) and/or generalize this struct (name)?
>
> However, our regulator setup appears to be different. I recall not
> finding any `vcca` supply in my downstream sources, and had this in my
> notes for a similar dsi_phy_14nm.c patch:
>
> sm6125 uses an RPM regulator
>
> https://github.com/sonyxperiadev/kernel/blob/f956fbd9a234033bd18234d456a2c32c126b38f3/arch/arm64/boot/dts/qcom/trinket-sde.dtsi#L388
I'd prefer a separate config for sm6125. This way you would be able to
add voting on the MX domain if required.
--
With best wishes
Dmitry
Re: [PATCH v3 2/2] drm/i915/uapi: expose GTT alignment
On 10/4/2022 1:49 PM, Matthew Auld wrote:
On some platforms we potentially have different alignment restrictions
depending on the memory type. We also now have different alignment
restrictions for the same region across different kernel versions.
Extend the region query to return the minimum required GTT alignment.
Testcase: igt@gem_create@create-ext-placement-alignment
Testcase: igt@i915_query@query-regions-sanity-check
Suggested-by: Lionel Landwerlin
Signed-off-by: Matthew Auld
Cc: Michal Mrozek
Cc: Thomas Hellström
Cc: Stuart Summers
Cc: Jordan Justen
Cc: Yang A Shi
Cc: Nirmoy Das
Reviewed-by: Nirmoy Das
Cc: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/i915_query.c | 1 +
include/uapi/drm/i915_drm.h | 29 +++--
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_query.c
b/drivers/gpu/drm/i915/i915_query.c
index 6ec9c9fb7b0d..111377f210ed 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -498,6 +498,7 @@ static int query_memregion_info(struct drm_i915_private
*i915,
info.region.memory_class = mr->type;
info.region.memory_instance = mr->instance;
info.probed_size = mr->total;
+ info.gtt_alignment = mr->min_page_size;
if (mr->type == INTEL_MEMORY_LOCAL)
info.probed_cpu_visible_size = mr->io_size;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 08d69e36fb66..2e613109356b 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3346,8 +3346,33 @@ struct drm_i915_memory_region_info {
/** @region: The class:instance pair encoding */
struct drm_i915_gem_memory_class_instance region;
- /** @rsvd0: MBZ */
- __u32 rsvd0;
+ union {
+ /** @rsvd0: MBZ */
+ __u32 rsvd0;
+ /**
+* @gtt_alignment:
+*
+* The minimum required GTT alignment for this type of memory.
+* When allocating a GTT address it must be aligned to this
+* value or larger. On some platforms the kernel might opt to
+* using 64K pages for I915_MEMORY_CLASS_DEVICE, where 64K GTT
+* pages can then be used if we also use 64K GTT alignment.
+*
+* NOTE: If this is zero then this must be an older
+* kernel which lacks support for this field.
+*
+* Side note: For larger objects (especially for
+* I915_MEMORY_CLASS_DEVICE), like 2M+ in size, userspace should
+* consider potentially bumping the GTT alignment to say 2M,
+* which could potentially increase the likelihood of the kernel
+* being able to utilise 2M GTT pages underneath, if the layout
+* of the physical pages allows it. On some configurations we
+* can then also use a more efficient page-table layout, if we
+* can't use the more desirable 2M GTT page, so long as we know
+* that the entire page-table will be used by this object.
+*/
+ __u32 gtt_alignment;
+ };
/**
* @probed_size: Memory probed by the driver
[PATCH] drm/sched: add missing NULL check in drm_sched_get_cleanup_job v2
Otherwise we would crash if the job is not resubmitted.
v2: fix second usage of s_fence->parent as well.
Signed-off-by: Christian König
---
drivers/gpu/drm/scheduler/sched_main.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index ce86b03e8386..4cc59bae38dd 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -919,7 +919,8 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
job = list_first_entry_or_null(>pending_list,
struct drm_sched_job, list);
- if (job && dma_fence_is_signaled(job->s_fence->parent)) {
+ if (job && (!job->s_fence->parent ||
+ dma_fence_is_signaled(job->s_fence->parent))) {
/* remove job from pending_list */
list_del_init(>list);
@@ -929,7 +930,7 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
next = list_first_entry_or_null(>pending_list,
typeof(*next), list);
- if (next) {
+ if (next && job->s_fence->parent) {
next->s_fence->scheduled.timestamp =
job->s_fence->parent->timestamp;
/* start TO timer for next job */
--
2.25.1
Re: [PATCH] dt-bindings: display: panel: use spi-peripheral-props.yaml
Hi Krzysztof,
On Tue, Oct 04, 2022 at 03:10:29PM +0200, Krzysztof Kozlowski wrote:
> On 04/10/2022 15:03, Laurent Pinchart wrote:
> > On Tue, Oct 04, 2022 at 02:09:07PM +0200, Krzysztof Kozlowski wrote:
> >> For devices connectable by SPI bus (e.g. already using
> >> "spi-max-frequency" property), reference the "spi-peripheral-props.yaml"
> >> schema to allow using all SPI device properties, even these which device
> >> bindings author did not tried yet.
> >
> > Isn't this done implicitly by spi-controller.yaml ? SPI devices that are
> > children of an SPI controller should match the patternProperties
> > "^.*@[0-9a-f]+$" in that file, which has a $ref: spi-peripheral-props.yaml.
> > Is there something I'm missing ?
>
> You are correct about one side of this - SPI controller bindings.
> However these schemas here have clear: additional/unevaluatedProperties:
> false, thus when they find DTS like:
> panel@xxx {
> compatible = "one of these spi panels";
> ...
> spi-cs-high;
> spi-rx-delay-us = <50>;
> ... and some more from specific controllers
> }
>
> you will get errors, because the panel schema does not allow them.
>
> The bindings were done (some time ago) in such way, that they require
> that both SPI controller and SPI device reference spi-props.
You're absolutely right that additionalProperties needs to be replaced
by unevaluatedProperties. Can the additions of $ref be dropped, or is
that needed too ?
--
Regards,
Laurent Pinchart
Re: [PATCH] dt-bindings: display: panel: use spi-peripheral-props.yaml
On 04/10/2022 15:03, Laurent Pinchart wrote:
> Hi Krzysztof,
>
> Thank you for the patch.
>
> On Tue, Oct 04, 2022 at 02:09:07PM +0200, Krzysztof Kozlowski wrote:
>> For devices connectable by SPI bus (e.g. already using
>> "spi-max-frequency" property), reference the "spi-peripheral-props.yaml"
>> schema to allow using all SPI device properties, even these which device
>> bindings author did not tried yet.
>
> Isn't this done implicitly by spi-controller.yaml ? SPI devices that are
> children of an SPI controller should match the patternProperties
> "^.*@[0-9a-f]+$" in that file, which has a $ref: spi-peripheral-props.yaml.
> Is there something I'm missing ?
>
You are correct about one side of this - SPI controller bindings.
However these schemas here have clear: additional/unevaluatedProperties:
false, thus when they find DTS like:
panel@xxx {
compatible = "one of these spi panels";
...
spi-cs-high;
spi-rx-delay-us = <50>;
... and some more from specific controllers
}
you will get errors, because the panel schema does not allow them.
The bindings were done (some time ago) in such way, that they require
that both SPI controller and SPI device reference spi-props.
Best regards,
Krzysztof
Re: [PATCH] drm/i915/pmu: Match frequencies reported by PMU and sysfs
On 04/10/2022 14:00, Tvrtko Ursulin wrote:
On 04/10/2022 10:29, Tvrtko Ursulin wrote:
On 03/10/2022 20:24, Ashutosh Dixit wrote:
PMU and sysfs use different wakeref's to "interpret" zero freq. Sysfs
uses
runtime PM wakeref (see intel_rps_read_punit_req and
intel_rps_read_actual_frequency). PMU uses the GT parked/unparked
wakeref. In general the GT wakeref is held for less time that the
runtime
PM wakeref which causes PMU to report a lower average freq than the
average
freq obtained from sampling sysfs.
To resolve this, use the same freq functions (and wakeref's) in PMU as
those used in sysfs.
Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7025
Reported-by: Ashwin Kumar Kulkarni
Cc: Tvrtko Ursulin
Signed-off-by: Ashutosh Dixit
---
drivers/gpu/drm/i915/i915_pmu.c | 27 ++-
1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c
b/drivers/gpu/drm/i915/i915_pmu.c
index 958b37123bf1..eda03f264792 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -371,37 +371,16 @@ static void
frequency_sample(struct intel_gt *gt, unsigned int period_ns)
{
struct drm_i915_private *i915 = gt->i915;
- struct intel_uncore *uncore = gt->uncore;
struct i915_pmu *pmu = >pmu;
struct intel_rps *rps = >rps;
if (!frequency_sampling_enabled(pmu))
return;
- /* Report 0/0 (actual/requested) frequency while parked. */
- if (!intel_gt_pm_get_if_awake(gt))
- return;
-
if (pmu->enable & config_mask(I915_PMU_ACTUAL_FREQUENCY)) {
- u32 val;
-
- /*
- * We take a quick peek here without using forcewake
- * so that we don't perturb the system under observation
- * (forcewake => !rc6 => increased power use). We expect
- * that if the read fails because it is outside of the
- * mmio power well, then it will return 0 -- in which
- * case we assume the system is running at the intended
- * frequency. Fortunately, the read should rarely fail!
- */
- val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1);
- if (val)
- val = intel_rps_get_cagf(rps, val);
- else
- val = rps->cur_freq;
-
add_sample_mult(>sample[__I915_SAMPLE_FREQ_ACT],
- intel_gpu_freq(rps, val), period_ns / 1000);
+ intel_rps_read_actual_frequency(rps),
+ period_ns / 1000);
}
if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) {
What is software tracking of requested frequency showing when GT is
parked or runtime suspended? With this change sampling would be
outside any such checks so we need to be sure reported value makes sense.
Although more important open is around what is actually correct.
For instance how does the patch affect RC6 and power? I don't know how
power management of different blocks is wired up, so personally I
would only be able to look at it empirically. In other words what I am
asking is this - if we changed from skipping obtaining forcewake even
when unparked, to obtaining forcewake if not runtime suspended - what
hardware blocks does that power up and how it affects RC6 and power?
Can it affect actual frequency or not? (Will "something" power up the
clocks just because we will be getting forcewake?)
Or maybe question simplified - does 200Hz polling on existing sysfs
actual frequency field disturbs the system under some circumstances?
(Increases power and decreases RC6.) If it does then that would be a
problem. We want a solution which shows the real data, but where the
act of monitoring itself does not change it too much. If it doesn't
then it's okay.
Could you somehow investigate on these topics? Maybe log RAPL GPU
power while polling on sysfs, versus getting the actual frequency from
the existing PMU implementation and see if that shows anything? Or
actually simpler - RAPL GPU power for current PMU intel_gpu_top versus
this patch? On idle(-ish) desktop workloads perhaps? Power and
frequency graphed for both.
Another thought - considering that bspec says for 0xa01c "This register
reflects real-time values and thus does not have a pre-determined
default value out of reset" - could it be that it also does not reflect
a real value when GPU is not executing anything (so zero), just happens
to be not runtime suspended? That would mean sysfs reads could maybe
show last known value? Just a thought to check.
I've also tried on my Alderlake desktop:
1)
while true; do cat gt_act_freq_mhz >/dev/null; sleep 0.005; done
This costs ~120mW of GPU power and ~20% decrease in RC6.
2)
intel_gpu_top -l -s 5 >/dev/null
This "-s 5" was pointless though. :)
Regards,
Tvrtko
This costs no power or RC6.
I have also never observed sysfs to show below min freq. This was with
no desktop so it's possible this register indeed does not reflect the
real situation when things
Re: [PATCH] dt-bindings: display: panel: use spi-peripheral-props.yaml
Hi Krzysztof, Thank you for the patch. On Tue, Oct 04, 2022 at 02:09:07PM +0200, Krzysztof Kozlowski wrote: > For devices connectable by SPI bus (e.g. already using > "spi-max-frequency" property), reference the "spi-peripheral-props.yaml" > schema to allow using all SPI device properties, even these which device > bindings author did not tried yet. Isn't this done implicitly by spi-controller.yaml ? SPI devices that are children of an SPI controller should match the patternProperties "^.*@[0-9a-f]+$" in that file, which has a $ref: spi-peripheral-props.yaml. Is there something I'm missing ? > Change "additionalProperties" to "unevaluatedProperties", so the actual > other properties from "spi-peripheral-props.yaml" can be used. This has > additional impact of allowing also other properties from > panel-common.yaml to be used. > > Signed-off-by: Krzysztof Kozlowski > --- > .../devicetree/bindings/display/panel/ilitek,ili9163.yaml| 3 ++- > .../devicetree/bindings/display/panel/ilitek,ili9341.yaml| 1 + > .../devicetree/bindings/display/panel/nec,nl8048hl11.yaml| 3 ++- > .../bindings/display/panel/samsung,lms380kf01.yaml | 5 ++--- > .../bindings/display/panel/samsung,lms397kf04.yaml | 3 ++- > .../devicetree/bindings/display/panel/samsung,s6d27a1.yaml | 4 ++-- > .../devicetree/bindings/display/panel/tpo,tpg110.yaml| 1 + > 7 files changed, 12 insertions(+), 8 deletions(-) > > diff --git > a/Documentation/devicetree/bindings/display/panel/ilitek,ili9163.yaml > b/Documentation/devicetree/bindings/display/panel/ilitek,ili9163.yaml > index 7e7a8362b951..a4154b51043e 100644 > --- a/Documentation/devicetree/bindings/display/panel/ilitek,ili9163.yaml > +++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9163.yaml > @@ -15,6 +15,7 @@ description: > > allOf: >- $ref: panel-common.yaml# > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > properties: >compatible: > @@ -41,7 +42,7 @@ required: >- dc-gpios >- reset-gpios > > -additionalProperties: false > +unevaluatedProperties: false > > examples: >- | > diff --git > a/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml > b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml > index 99e0cb9440cf..94f169ea065a 100644 > --- a/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml > +++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml > @@ -16,6 +16,7 @@ description: | > > allOf: >- $ref: panel-common.yaml# > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > properties: >compatible: > diff --git > a/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml > b/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml > index aa788eaa2f71..3b09b359023e 100644 > --- a/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml > +++ b/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml > @@ -15,6 +15,7 @@ maintainers: > > allOf: >- $ref: panel-common.yaml# > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > properties: >compatible: > @@ -34,7 +35,7 @@ required: >- reset-gpios >- port > > -additionalProperties: false > +unevaluatedProperties: false > > examples: >- | > diff --git > a/Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml > b/Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml > index 251f0c7115aa..70ffc88d2a08 100644 > --- a/Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml > +++ b/Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml > @@ -9,14 +9,13 @@ title: Samsung LMS380KF01 display panel > description: The LMS380KF01 is a 480x800 DPI display panel from Samsung > Mobile >Displays (SMD) utilizing the WideChips WS2401 display controller. It can be >used with internal or external backlight control. > - The panel must obey the rules for a SPI slave device as specified in > - spi/spi-controller.yaml > > maintainers: >- Linus Walleij > > allOf: >- $ref: panel-common.yaml# > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > properties: >compatible: > @@ -59,7 +58,7 @@ required: >- spi-cpol >- port > > -additionalProperties: false > +unevaluatedProperties: false > > examples: >- | > diff --git > a/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml > b/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml > index cd62968426fb..5e77cee93f83 100644 > --- a/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml > +++ b/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml > @@ -14,6 +14,7 @@ maintainers: > > allOf: >- $ref: panel-common.yaml# > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > > properties: >compatible: > @@ -51,7 +52,7 @@ required: >- spi-cpol >-
Re: [PATCH v2 12/14] drm/i915: Define multicast registers as a new type
On Tue, 04 Oct 2022, Jani Nikula wrote:
> On Fri, 30 Sep 2022, Matt Roper wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h
>> b/drivers/gpu/drm/i915/i915_reg_defs.h
>> index 8f486f77609f..e823869b9afd 100644
>> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
>> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
>> @@ -104,22 +104,16 @@ typedef struct {
>>
>> #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
>>
>> -#define INVALID_MMIO_REG _MMIO(0)
>> -
>> -static __always_inline u32 i915_mmio_reg_offset(i915_reg_t reg)
>> -{
>> -return reg.reg;
>> -}
>> +typedef struct {
>> +u32 reg;
>> +} i915_mcr_reg_t;
>>
>> -static inline bool i915_mmio_reg_equal(i915_reg_t a, i915_reg_t b)
>> -{
>> -return i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b);
>> -}
>> +#define INVALID_MMIO_REG _MMIO(0)
>>
>> -static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>> -{
>> -return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
>> -}
>> +/* These macros can be used on either i915_reg_t or i915_mcr_reg_t */
>> +#define i915_mmio_reg_offset(r) (r.reg)
>> +#define i915_mmio_reg_equal(a, b) (i915_mmio_reg_offset(a) ==
>> i915_mmio_reg_offset(b))
>> +#define i915_mmio_reg_valid(r) (!i915_mmio_reg_equal(r, INVALID_MMIO_REG))
>>
>
> I don't really like losing the type safety here. The whole and only
> purpose of typedeffing i915_reg_t as a struct instead of just u32 was
> the strict type safety.
PS. Changes like this should really be highlighted better, in the commit
subject and title. Now it feels like it's hidden within a big commit
within a big series, without even mentioning it in the commit message.
BR,
Jani.
>
> BR,
> Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH] drm/i915/pmu: Match frequencies reported by PMU and sysfs
On 04/10/2022 10:29, Tvrtko Ursulin wrote:
On 03/10/2022 20:24, Ashutosh Dixit wrote:
PMU and sysfs use different wakeref's to "interpret" zero freq. Sysfs
uses
runtime PM wakeref (see intel_rps_read_punit_req and
intel_rps_read_actual_frequency). PMU uses the GT parked/unparked
wakeref. In general the GT wakeref is held for less time that the runtime
PM wakeref which causes PMU to report a lower average freq than the
average
freq obtained from sampling sysfs.
To resolve this, use the same freq functions (and wakeref's) in PMU as
those used in sysfs.
Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7025
Reported-by: Ashwin Kumar Kulkarni
Cc: Tvrtko Ursulin
Signed-off-by: Ashutosh Dixit
---
drivers/gpu/drm/i915/i915_pmu.c | 27 ++-
1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c
b/drivers/gpu/drm/i915/i915_pmu.c
index 958b37123bf1..eda03f264792 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -371,37 +371,16 @@ static void
frequency_sample(struct intel_gt *gt, unsigned int period_ns)
{
struct drm_i915_private *i915 = gt->i915;
- struct intel_uncore *uncore = gt->uncore;
struct i915_pmu *pmu = >pmu;
struct intel_rps *rps = >rps;
if (!frequency_sampling_enabled(pmu))
return;
- /* Report 0/0 (actual/requested) frequency while parked. */
- if (!intel_gt_pm_get_if_awake(gt))
- return;
-
if (pmu->enable & config_mask(I915_PMU_ACTUAL_FREQUENCY)) {
- u32 val;
-
- /*
- * We take a quick peek here without using forcewake
- * so that we don't perturb the system under observation
- * (forcewake => !rc6 => increased power use). We expect
- * that if the read fails because it is outside of the
- * mmio power well, then it will return 0 -- in which
- * case we assume the system is running at the intended
- * frequency. Fortunately, the read should rarely fail!
- */
- val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1);
- if (val)
- val = intel_rps_get_cagf(rps, val);
- else
- val = rps->cur_freq;
-
add_sample_mult(>sample[__I915_SAMPLE_FREQ_ACT],
- intel_gpu_freq(rps, val), period_ns / 1000);
+ intel_rps_read_actual_frequency(rps),
+ period_ns / 1000);
}
if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) {
What is software tracking of requested frequency showing when GT is
parked or runtime suspended? With this change sampling would be outside
any such checks so we need to be sure reported value makes sense.
Although more important open is around what is actually correct.
For instance how does the patch affect RC6 and power? I don't know how
power management of different blocks is wired up, so personally I would
only be able to look at it empirically. In other words what I am asking
is this - if we changed from skipping obtaining forcewake even when
unparked, to obtaining forcewake if not runtime suspended - what
hardware blocks does that power up and how it affects RC6 and power? Can
it affect actual frequency or not? (Will "something" power up the clocks
just because we will be getting forcewake?)
Or maybe question simplified - does 200Hz polling on existing sysfs
actual frequency field disturbs the system under some circumstances?
(Increases power and decreases RC6.) If it does then that would be a
problem. We want a solution which shows the real data, but where the act
of monitoring itself does not change it too much. If it doesn't then
it's okay.
Could you somehow investigate on these topics? Maybe log RAPL GPU power
while polling on sysfs, versus getting the actual frequency from the
existing PMU implementation and see if that shows anything? Or actually
simpler - RAPL GPU power for current PMU intel_gpu_top versus this
patch? On idle(-ish) desktop workloads perhaps? Power and frequency
graphed for both.
Another thought - considering that bspec says for 0xa01c "This register
reflects real-time values and thus does not have a pre-determined
default value out of reset" - could it be that it also does not reflect
a real value when GPU is not executing anything (so zero), just happens
to be not runtime suspended? That would mean sysfs reads could maybe
show last known value? Just a thought to check.
I've also tried on my Alderlake desktop:
1)
while true; do cat gt_act_freq_mhz >/dev/null; sleep 0.005; done
This costs ~120mW of GPU power and ~20% decrease in RC6.
2)
intel_gpu_top -l -s 5 >/dev/null
This costs no power or RC6.
I have also never observed sysfs to show below min freq. This was with
no desktop so it's possible this register indeed does not reflect the
real situation when things are idle.
So I think it is possible sysfs value is the misleading one.
Regards,
Tvrtko
Re: [PATCH v2 12/14] drm/i915: Define multicast registers as a new type
On Fri, 30 Sep 2022, Matt Roper wrote:
> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h
> b/drivers/gpu/drm/i915/i915_reg_defs.h
> index 8f486f77609f..e823869b9afd 100644
> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
> @@ -104,22 +104,16 @@ typedef struct {
>
> #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
>
> -#define INVALID_MMIO_REG _MMIO(0)
> -
> -static __always_inline u32 i915_mmio_reg_offset(i915_reg_t reg)
> -{
> - return reg.reg;
> -}
> +typedef struct {
> + u32 reg;
> +} i915_mcr_reg_t;
>
> -static inline bool i915_mmio_reg_equal(i915_reg_t a, i915_reg_t b)
> -{
> - return i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b);
> -}
> +#define INVALID_MMIO_REG _MMIO(0)
>
> -static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> -{
> - return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
> -}
> +/* These macros can be used on either i915_reg_t or i915_mcr_reg_t */
> +#define i915_mmio_reg_offset(r) (r.reg)
> +#define i915_mmio_reg_equal(a, b) (i915_mmio_reg_offset(a) ==
> i915_mmio_reg_offset(b))
> +#define i915_mmio_reg_valid(r) (!i915_mmio_reg_equal(r, INVALID_MMIO_REG))
>
I don't really like losing the type safety here. The whole and only
purpose of typedeffing i915_reg_t as a struct instead of just u32 was
the strict type safety.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [RFC/PATCH] backlight: hx8357: prepare to conversion to gpiod API
On Tue, Oct 04, 2022 at 11:02:06AM +0200, Linus Walleij wrote: > On Wed, Sep 28, 2022 at 12:32 AM Dmitry Torokhov > wrote: > > > Properties describing GPIOs should be named as "-gpios" or > > "-gpio", and that is what gpiod API expects, however the > > driver uses non-standard "gpios-reset" name. Let's adjust this, and also > > note that the reset line is active low as that is also important to > > gpiod API. > > > > Signed-off-by: Dmitry Torokhov > > I think the gods of Open Firmware will try to punish you for such > incompatible changes. But I have long since renounced them. > > > Another option is to add another quirk into gpiolib-of.c, but we > > may end up with a ton of them once we convert everything away from > > of_get_named_gpio() to gpiod API, so I'd prefer not doing that. > > We need to know if i.MX is shipping device trees stored in flash, > or if they bundle it with the kernel. This part is frequently found in add-on boards so it's not purely an i.MX-only question. > In the former case, you have to add quirks, in the latter case this > patch is fine. > > Sascha, what does the Freescale maintainer say? IMHO for not-in-the-soc devices like this the presence of in-kernel DTs isn't enough to make a decision. What is needed is a degree of due-diligence to show that there are no obvious out-of-kernel users. To be honest, I suspect the due-diligence checks will probably yield a green light for this one. Most of the tutorials for the popular HX8357 devices, show how to run python code in userspace that sends raw SPI commands. That sucks but at least it doesn't raise any concerns about bindings maintenance. Daniel.
Re: [RFC PATCH 1/1] drm/bridge: ti-sn65dsi83: Remove burst mode
On 10/4/22 13:57, e...@gmx.net wrote:
From: Eberhard Stoll
Remove LVDS panel overclocking for some configurations by disabling
burst mode for this chip
With burst mode enabled, some DSI controllers increase their DSI lane
clock beyond the clock which is needed to transfer all pixel data.
But this driver operates with a pixel clock derived from the DSI lane
clock by a fixed prescaler. So, every increase of the DSI clock also
increases the LVDS pixel clock. In this case, the LVDS pixel clock is
overclocked.
This is the case e.g. for synopsys stm DSI bridge
(drm/stm/dw_mipi_dsi-stm.c). With burst mode the DSI clock is
increased by 20% and therefore also for the LVDS panel.
Signed-off-by: Eberhard Stoll
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index c901c0e1a3b0..ffc39208536e 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -272,7 +272,7 @@ static int sn65dsi83_attach(struct drm_bridge *bridge,
dsi->lanes = ctx->dsi_lanes;
dsi->format = MIPI_DSI_FMT_RGB888;
- dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST;
+ dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
ret = mipi_dsi_attach(dsi);
if (ret < 0) {
The burst mode is the most efficient mode, so please keep it. This patch
is the wrong approach.
The problem with DSI HS clock is known and long standing, I tried to
solve it twice, but neither solution made it in. It is probably a good
idea to read through the suggestions and try to follow the latest one by
negotiating the link clock via bridge state:
https://lore.kernel.org/dri-devel/20220801131113.182487-1-ma...@denx.de/
https://lore.kernel.org/all/20220219002844.362157-1-ma...@denx.de/
[PATCH] dt-bindings: display: panel: use spi-peripheral-props.yaml
For devices connectable by SPI bus (e.g. already using "spi-max-frequency" property), reference the "spi-peripheral-props.yaml" schema to allow using all SPI device properties, even these which device bindings author did not tried yet. Change "additionalProperties" to "unevaluatedProperties", so the actual other properties from "spi-peripheral-props.yaml" can be used. This has additional impact of allowing also other properties from panel-common.yaml to be used. Signed-off-by: Krzysztof Kozlowski --- .../devicetree/bindings/display/panel/ilitek,ili9163.yaml| 3 ++- .../devicetree/bindings/display/panel/ilitek,ili9341.yaml| 1 + .../devicetree/bindings/display/panel/nec,nl8048hl11.yaml| 3 ++- .../bindings/display/panel/samsung,lms380kf01.yaml | 5 ++--- .../bindings/display/panel/samsung,lms397kf04.yaml | 3 ++- .../devicetree/bindings/display/panel/samsung,s6d27a1.yaml | 4 ++-- .../devicetree/bindings/display/panel/tpo,tpg110.yaml| 1 + 7 files changed, 12 insertions(+), 8 deletions(-) diff --git a/Documentation/devicetree/bindings/display/panel/ilitek,ili9163.yaml b/Documentation/devicetree/bindings/display/panel/ilitek,ili9163.yaml index 7e7a8362b951..a4154b51043e 100644 --- a/Documentation/devicetree/bindings/display/panel/ilitek,ili9163.yaml +++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9163.yaml @@ -15,6 +15,7 @@ description: allOf: - $ref: panel-common.yaml# + - $ref: /schemas/spi/spi-peripheral-props.yaml# properties: compatible: @@ -41,7 +42,7 @@ required: - dc-gpios - reset-gpios -additionalProperties: false +unevaluatedProperties: false examples: - | diff --git a/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml index 99e0cb9440cf..94f169ea065a 100644 --- a/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml +++ b/Documentation/devicetree/bindings/display/panel/ilitek,ili9341.yaml @@ -16,6 +16,7 @@ description: | allOf: - $ref: panel-common.yaml# + - $ref: /schemas/spi/spi-peripheral-props.yaml# properties: compatible: diff --git a/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml b/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml index aa788eaa2f71..3b09b359023e 100644 --- a/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml +++ b/Documentation/devicetree/bindings/display/panel/nec,nl8048hl11.yaml @@ -15,6 +15,7 @@ maintainers: allOf: - $ref: panel-common.yaml# + - $ref: /schemas/spi/spi-peripheral-props.yaml# properties: compatible: @@ -34,7 +35,7 @@ required: - reset-gpios - port -additionalProperties: false +unevaluatedProperties: false examples: - | diff --git a/Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml b/Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml index 251f0c7115aa..70ffc88d2a08 100644 --- a/Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml +++ b/Documentation/devicetree/bindings/display/panel/samsung,lms380kf01.yaml @@ -9,14 +9,13 @@ title: Samsung LMS380KF01 display panel description: The LMS380KF01 is a 480x800 DPI display panel from Samsung Mobile Displays (SMD) utilizing the WideChips WS2401 display controller. It can be used with internal or external backlight control. - The panel must obey the rules for a SPI slave device as specified in - spi/spi-controller.yaml maintainers: - Linus Walleij allOf: - $ref: panel-common.yaml# + - $ref: /schemas/spi/spi-peripheral-props.yaml# properties: compatible: @@ -59,7 +58,7 @@ required: - spi-cpol - port -additionalProperties: false +unevaluatedProperties: false examples: - | diff --git a/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml b/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml index cd62968426fb..5e77cee93f83 100644 --- a/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml +++ b/Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml @@ -14,6 +14,7 @@ maintainers: allOf: - $ref: panel-common.yaml# + - $ref: /schemas/spi/spi-peripheral-props.yaml# properties: compatible: @@ -51,7 +52,7 @@ required: - spi-cpol - port -additionalProperties: false +unevaluatedProperties: false examples: - | diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6d27a1.yaml b/Documentation/devicetree/bindings/display/panel/samsung,s6d27a1.yaml index 26e3c820a2f7..d273faf4442a 100644 --- a/Documentation/devicetree/bindings/display/panel/samsung,s6d27a1.yaml +++ b/Documentation/devicetree/bindings/display/panel/samsung,s6d27a1.yaml @@ -7,14 +7,14 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# title: Samsung S6D27A1 display panel description: The S6D27A1 is a 480x800 DPI display panel from Samsung
Re: [PATCH v1 17/17] drm/mediatek: Add mt8195-dpi support to drm_drv
On Tue, 04 Oct 2022 12:49, Krzysztof Kozlowski
wrote:
>On 03/10/2022 17:29, Guillaume Ranquet wrote:
>> On Tue, 27 Sep 2022 16:28, Krzysztof Kozlowski
>> wrote:
>>> On 27/09/2022 15:04, Guillaume Ranquet wrote:
On Thu, 22 Sep 2022 09:20, Krzysztof Kozlowski
wrote:
> On 19/09/2022 18:56, Guillaume Ranquet wrote:
>> Add dpi support to enable the HDMI path.
>>
>> Signed-off-by: Guillaume Ranquet
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> index 72049a530ae1..27f029ca760b 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> @@ -820,6 +820,8 @@ static const struct of_device_id
>> mtk_ddp_comp_dt_ids[] = {
>>.data = (void *)MTK_DPI },
>> { .compatible = "mediatek,mt8192-dpi",
>>.data = (void *)MTK_DPI },
>> +{ .compatible = "mediatek,mt8195-dpi",
>> + .data = (void *)MTK_DPI },
>
> It's compatible with the others. You don't need more compatibles.
Hi Krzysztof,
It's a bit confusing, because this compatible is used in both
mtk_drm_drv.c and in mtk_dpi.c
Albeit it's entirely the same thing regarding the mtk_drm_drv module,
it's pretty different
regarding the mtk_dpi module.
>>>
>>> Sure, but this does not explain why do you need these entries here in
>>> mtk_drm_drv.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Hi Krzysztof,
>>
>> Sorry for the late answer.
>> The mtk_drm_drv is the component master of the full mediatek drm stack.
>>
>> it "binds" all of the crtc/dpi/ovl/mutex/merge... components of the stack.
>>
>> That mtk_ddp_comp_dt_ids array is iterated over to find all of the components
>> from the device tree.
>
>No. You said what the code is doing. I think I understand this. You
>still do not need more compatibles. Your sentence did not clarify it
>because it did not answer at all to question "why". Why do you need it?
>
>Sorry, the change looks not correct.
>
>Best regards,
>Krzysztof
>
I need a new compatible to adress the specifics of mt8195 in the mtk_dpi driver,
the change is in this series with:
[PATCH v1 16/17] drm/mediatek: dpi: Add mt8195 hdmi to DPI driver [1]
I then need to add that compatible to the "list" here in mtk_drm_drv.
I don't see a way around this unless I rewrite the way mtk_drm_drv works?
Maybe if I declare a new compatible that is generic to all mediatek
dpi variants?
and have all the dts specify the node with both the generic dpi and
the specific compatible?
dpi@xxx {
compatible = "mediatek,dpi", "mediatek,mt8195-dpi";
...
}
Then I can "collapse" all the dpi related nodes in mtk_drm_drv under
"mediatek,dpi" ?
I guess would have to do the change for all other components that are needed in
mtk_drm_drv (mmsys, aal, ccor, color, dither, dsc, gamma, mutex...).
That's the only trivial way I can think of implementing this with the
current status
of the mtk_drm stack.
Do you have any other ideas in mind?
Thx,
Guillaume.
[1] : https://lore.kernel.org/all/20220919-v1-16-4844816c9...@baylibre.com/
[PATCH v3 2/2] drm/i915/uapi: expose GTT alignment
On some platforms we potentially have different alignment restrictions
depending on the memory type. We also now have different alignment
restrictions for the same region across different kernel versions.
Extend the region query to return the minimum required GTT alignment.
Testcase: igt@gem_create@create-ext-placement-alignment
Testcase: igt@i915_query@query-regions-sanity-check
Suggested-by: Lionel Landwerlin
Signed-off-by: Matthew Auld
Cc: Michal Mrozek
Cc: Thomas Hellström
Cc: Stuart Summers
Cc: Jordan Justen
Cc: Yang A Shi
Cc: Nirmoy Das
Cc: Niranjana Vishwanathapura
---
drivers/gpu/drm/i915/i915_query.c | 1 +
include/uapi/drm/i915_drm.h | 29 +++--
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_query.c
b/drivers/gpu/drm/i915/i915_query.c
index 6ec9c9fb7b0d..111377f210ed 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -498,6 +498,7 @@ static int query_memregion_info(struct drm_i915_private
*i915,
info.region.memory_class = mr->type;
info.region.memory_instance = mr->instance;
info.probed_size = mr->total;
+ info.gtt_alignment = mr->min_page_size;
if (mr->type == INTEL_MEMORY_LOCAL)
info.probed_cpu_visible_size = mr->io_size;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 08d69e36fb66..2e613109356b 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3346,8 +3346,33 @@ struct drm_i915_memory_region_info {
/** @region: The class:instance pair encoding */
struct drm_i915_gem_memory_class_instance region;
- /** @rsvd0: MBZ */
- __u32 rsvd0;
+ union {
+ /** @rsvd0: MBZ */
+ __u32 rsvd0;
+ /**
+* @gtt_alignment:
+*
+* The minimum required GTT alignment for this type of memory.
+* When allocating a GTT address it must be aligned to this
+* value or larger. On some platforms the kernel might opt to
+* using 64K pages for I915_MEMORY_CLASS_DEVICE, where 64K GTT
+* pages can then be used if we also use 64K GTT alignment.
+*
+* NOTE: If this is zero then this must be an older
+* kernel which lacks support for this field.
+*
+* Side note: For larger objects (especially for
+* I915_MEMORY_CLASS_DEVICE), like 2M+ in size, userspace should
+* consider potentially bumping the GTT alignment to say 2M,
+* which could potentially increase the likelihood of the kernel
+* being able to utilise 2M GTT pages underneath, if the layout
+* of the physical pages allows it. On some configurations we
+* can then also use a more efficient page-table layout, if we
+* can't use the more desirable 2M GTT page, so long as we know
+* that the entire page-table will be used by this object.
+*/
+ __u32 gtt_alignment;
+ };
/**
* @probed_size: Memory probed by the driver
--
2.37.3
[PATCH v3 1/2] drm/i915: enable PS64 support for DG2
It turns out that on production DG2/ATS HW we should have support for
PS64. This feature allows to provide a 64K TLB hint at the PTE level,
which is a lot more flexible than the current method of enabling 64K GTT
pages for the entire page-table, since that leads to all kinds of
annoying restrictions, as documented in:
commit caa574ffc4aaf4f29b890223878c63e2e7772f62
Author: Matthew Auld
Date: Sat Feb 19 00:17:49 2022 +0530
drm/i915/uapi: document behaviour for DG2 64K support
On discrete platforms like DG2, we need to support a minimum page size
of 64K when dealing with device local-memory. This is quite tricky for
various reasons, so try to document the new implicit uapi for this.
With PS64, we can now drop the 2M GTT alignment restriction, and instead
only require 64K or larger when dealing with lmem. We still use the
compact-pt layout when possible, but only when we are certain that this
doesn't interfere with userspace.
Note that this is a change in uAPI behaviour, but hopefully shouldn't be
a concern (IGT is at least able to autodetect the alignment), since we
are only making the GTT alignment constraint less restrictive.
Based on a patch from CQ Tang.
v2: update the comment wrt scratch page
v3: (Nirmoy)
- Fix the selftest to actually use the random size, plus some comment
improvements, also drop the rem stuff.
Reported-by: Michal Mrozek
Signed-off-by: Matthew Auld
Cc: Lionel Landwerlin
Cc: Thomas Hellström
Cc: Stuart Summers
Cc: Jordan Justen
Cc: Yang A Shi
Cc: Nirmoy Das
Cc: Niranjana Vishwanathapura
Reviewed-by: Nirmoy Das
Acked-by: Michal Mrozek
---
.../gpu/drm/i915/gem/selftests/huge_pages.c | 157 +-
drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 81 +
drivers/gpu/drm/i915/gt/intel_gtt.c | 21 +--
drivers/gpu/drm/i915/gt/intel_gtt.h | 1 +
drivers/gpu/drm/i915/i915_drv.h | 7 -
drivers/gpu/drm/i915/i915_pci.c | 2 -
drivers/gpu/drm/i915/i915_vma.c | 9 +-
drivers/gpu/drm/i915/intel_device_info.h | 1 -
drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 9 +-
include/uapi/drm/i915_drm.h | 36 ++--
10 files changed, 218 insertions(+), 106 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index c570cf780079..0cb99e75b0bc 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -1161,7 +1161,8 @@ static int igt_write_huge(struct drm_i915_private *i915,
GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
size = obj->base.size;
- if (obj->mm.page_sizes.sg & I915_GTT_PAGE_SIZE_64K)
+ if (obj->mm.page_sizes.sg & I915_GTT_PAGE_SIZE_64K &&
+ !HAS_64K_PAGES(i915))
size = round_up(size, I915_GTT_PAGE_SIZE_2M);
n = 0;
@@ -1214,6 +1215,10 @@ static int igt_write_huge(struct drm_i915_private *i915,
* size and ensure the vma offset is at the start of the pt
* boundary, however to improve coverage we opt for testing both
* aligned and unaligned offsets.
+*
+* With PS64 this is no longer the case, but to ensure we
+* sometimes get the compact layout for smaller objects, apply
+* the round_up anyway.
*/
if (obj->mm.page_sizes.sg & I915_GTT_PAGE_SIZE_64K)
offset_low = round_down(offset_low,
@@ -1411,6 +1416,7 @@ static int igt_ppgtt_sanity_check(void *arg)
{ SZ_2M + SZ_4K,SZ_64K | SZ_4K },
{ SZ_2M + SZ_4K,SZ_2M | SZ_4K },
{ SZ_2M + SZ_64K, SZ_2M | SZ_64K },
+ { SZ_2M + SZ_64K, SZ_64K },
};
int i, j;
int err;
@@ -1540,6 +1546,154 @@ static int igt_ppgtt_compact(void *arg)
return err;
}
+static int igt_ppgtt_mixed(void *arg)
+{
+ struct drm_i915_private *i915 = arg;
+ const unsigned long flags = PIN_OFFSET_FIXED | PIN_USER;
+ struct drm_i915_gem_object *obj, *on;
+ struct i915_gem_engines *engines;
+ struct i915_gem_engines_iter it;
+ struct i915_address_space *vm;
+ struct i915_gem_context *ctx;
+ struct intel_context *ce;
+ struct file *file;
+ I915_RND_STATE(prng);
+ LIST_HEAD(objects);
+ struct intel_memory_region *mr;
+ struct i915_vma *vma;
+ unsigned int count;
+ u32 i, addr;
+ int *order;
+ int n, err;
+
+ /*
+* Sanity check mixing 4K and 64K pages within the same page-table via
+* the new PS64 TLB hint.
+*/
+
+ if (!HAS_64K_PAGES(i915)) {
+ pr_info("device lacks PS64, skipping\n");
+ return 0;
+ }
+
+ file = mock_file(i915);
+ if (IS_ERR(file))
+
Re: linux-next: build failure after merge of the drm tree
On Tue, Oct 04, 2022 at 02:05:58PM +1100, Stephen Rothwell wrote: > On Tue, 4 Oct 2022 12:24:37 +1000 David Airlie wrote: > > On Tue, Oct 4, 2022 at 12:21 PM Stephen Rothwell > > wrote: > > I'm not seeing it here, what gcc is this with? > I am using an x86_64 cross compiler hosted on ppc64le - gcc v11.2.0 (on > Debian). I was seeing this with an x86_64 cross compiler hosted on arm64 - Ubuntu 11.2.0-17ubuntu1 from the looks of it. signature.asc Description: PGP signature
Re: [Intel-gfx] [PATCH v2] drm/i915/guc: Fix revocation of non-persistent contexts
On 03/10/2022 13:16, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Patch which added graceful exit for non-persistent contexts missed the
fact it is not enough to set the exiting flag on a context and let the
backend handle it from there.
GuC backend cannot handle it because it runs independently in the
firmware and driver might not see the requests ever again. Patch also
missed the fact some usages of intel_context_is_banned in the GuC backend
needed replacing with newly introduced intel_context_is_schedulable.
Fix the first issue by calling into backend revoke when we know this is
the last chance to do it. Fix the second issue by replacing
intel_context_is_banned with intel_context_is_schedulable, which should
always be safe since latter is a superset of the former.
v2:
* Just call ce->ops->revoke unconditionally. (Andrzej)
CI is happy - could I get some acks for the GuC backend changes please?
Regards,
Tvrtko
Signed-off-by: Tvrtko Ursulin
Fixes: 45c64ecf97ee ("drm/i915: Improve user experience and driver robustness under
SIGINT or similar")
Cc: Andrzej Hajda
Cc: John Harrison
Cc: Daniele Ceraolo Spurio
Cc: # v6.0+
Reviewed-by: Andrzej Hajda
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 8 +-
drivers/gpu/drm/i915/gt/intel_context.c | 5 ++--
drivers/gpu/drm/i915/gt/intel_context.h | 3 +--
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 26 +--
4 files changed, 17 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 0bcde53c50c6..1e29b1e6d186 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1387,14 +1387,8 @@ kill_engines(struct i915_gem_engines *engines, bool
exit, bool persistent)
*/
for_each_gem_engine(ce, engines, it) {
struct intel_engine_cs *engine;
- bool skip = false;
- if (exit)
- skip = intel_context_set_exiting(ce);
- else if (!persistent)
- skip = intel_context_exit_nonpersistent(ce, NULL);
-
- if (skip)
+ if ((exit || !persistent) && intel_context_revoke(ce))
continue; /* Already marked. */
/*
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index 654a092ed3d6..e94365b08f1e 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -614,13 +614,12 @@ bool intel_context_ban(struct intel_context *ce, struct
i915_request *rq)
return ret;
}
-bool intel_context_exit_nonpersistent(struct intel_context *ce,
- struct i915_request *rq)
+bool intel_context_revoke(struct intel_context *ce)
{
bool ret = intel_context_set_exiting(ce);
if (ce->ops->revoke)
- ce->ops->revoke(ce, rq, ce->engine->props.preempt_timeout_ms);
+ ce->ops->revoke(ce, NULL, ce->engine->props.preempt_timeout_ms);
return ret;
}
diff --git a/drivers/gpu/drm/i915/gt/intel_context.h
b/drivers/gpu/drm/i915/gt/intel_context.h
index 8e2d70630c49..be09fb2e883a 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.h
+++ b/drivers/gpu/drm/i915/gt/intel_context.h
@@ -329,8 +329,7 @@ static inline bool intel_context_set_exiting(struct
intel_context *ce)
return test_and_set_bit(CONTEXT_EXITING, >flags);
}
-bool intel_context_exit_nonpersistent(struct intel_context *ce,
- struct i915_request *rq);
+bool intel_context_revoke(struct intel_context *ce);
static inline bool
intel_context_force_single_submission(const struct intel_context *ce)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 0ef295a94060..88a4476b8e92 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -685,7 +685,7 @@ static int __guc_add_request(struct intel_guc *guc, struct
i915_request *rq)
* Corner case where requests were sitting in the priority list or a
* request resubmitted after the context was banned.
*/
- if (unlikely(intel_context_is_banned(ce))) {
+ if (unlikely(!intel_context_is_schedulable(ce))) {
i915_request_put(i915_request_mark_eio(rq));
intel_engine_signal_breadcrumbs(ce->engine);
return 0;
@@ -871,15 +871,15 @@ static int guc_wq_item_append(struct intel_guc *guc,
struct i915_request *rq)
{
struct intel_context *ce = request_to_scheduling_context(rq);
- int ret = 0;
+ int ret;
- if (likely(!intel_context_is_banned(ce))) {
- ret = __guc_wq_item_append(rq);
+ if (unlikely(!intel_context_is_schedulable(ce)))
+ return 0;
Re: [PATCH 1/4] dt-bindings: display: Add bindings for JDI LPM102A188A
On 03/10/2022 19:06, Diogo Ivo wrote: > On Fri, Sep 30, 2022 at 12:49:31PM +0200, Krzysztof Kozlowski wrote: >>> + ts-reset-gpios: >>> +maxItems: 1 >>> +description: | >>> + Specifier for a GPIO connected to the touchscreen reset control >>> signal. >>> + The reset signal is active low. >> >> Isn't touchscreen a separate (input) device? > > Hello, thank you for the feedback. > > According to the downstream kernel's log, it seems like the panel and > the touchscreen controller are considered to be embedded in the same unit > (for example in [1]), Downstream kernel is not a proof of proper description of hardware. If downstream says orange is an apple, does it mean orange is really an apple? No... Downstream creates a lot of junk, hacks and workarounds. > with the touch input being transmitted via HID-over-I2C, > and since I did not find any reset gpio handling in that driver I opted to > include this reset here, unless there is a better way of going about this. Instead it should be in touch screen device. > > Best regards, > > Diogo > > [1]: > https://android.googlesource.com/kernel/tegra/+/bca61c34db9f72113af058f53eeb9fbd5e69a1d0 Where is the DTS of that device? Best regards, Krzysztof
Re: [PATCH 2/2] drm/i915: remove circ_buf.h includes
On Tue, 04 Oct 2022, "Jiri Slaby (SUSE)" wrote:
> The last user of macros from that include was removed in 2018 by the
> commit below.
>
> Fixes: 6cc42152b02b ("drm/i915: Remove support for legacy debugfs crc
> interface")
> Cc: Jani Nikula
> Cc: Joonas Lahtinen
> Cc: Rodrigo Vivi
> Cc: Tvrtko Ursulin
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Jiri Slaby (SUSE)
Reviewed-by: Jani Nikula
> ---
> drivers/gpu/drm/i915/display/intel_pipe_crc.c | 1 -
> drivers/gpu/drm/i915/i915_irq.c | 1 -
> 2 files changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_pipe_crc.c
> b/drivers/gpu/drm/i915/display/intel_pipe_crc.c
> index 8ac263f471be..9070935b0443 100644
> --- a/drivers/gpu/drm/i915/display/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/display/intel_pipe_crc.c
> @@ -24,7 +24,6 @@
> *
> */
>
> -#include
> #include
> #include
> #include
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 86a42d9e8041..09d728b34a47 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -28,7 +28,6 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> -#include
> #include
> #include
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 1/2] drm/i915/display: fix randconfig build
On Tue, 04 Oct 2022, "Jiri Slaby (SUSE)" wrote:
> When DRM_I915=y and BACKLIGHT_CLASS_DEVICE=m, the build fails:
> ld: drivers/gpu/drm/i915/display/intel_backlight.o: in function
> `intel_backlight_device_register':
> intel_backlight.c:(.text+0x5587): undefined reference to
> `backlight_device_get_by_name'
>
> ld: drivers/gpu/drm/i915/display/intel_backlight.o: in function
> `intel_backlight_device_unregister':
> intel_backlight.c:(.text+0x576e): undefined reference to
> `backlight_device_unregister'
>
> To fix this, use IS_REACHABLE(), not IS_ENABLED() in backlight. That is,
> with the above config, backlight support is disabled.
So I don't want this. I'll take a patch that fixes the dependencies to
block DRM_I915=y and BACKLIGHT_CLASS_DEVICE=m. Nobody wants that combo,
and IMO using IS_REACHABLE() is a workaround to hide a broken config
under the carpet.
The right thing to do is
config DRM_I915
depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n.
We're selecting BACKLIGHT_CLASS_DEVICE because almost everyone else is
too, and a combo of selecting and depending leads to circular
dependencies. But depending is the right fix.
Documentation/kbuild/kconfig-language.rst:
Note:
select should be used with care. select will force
a symbol to a value without visiting the dependencies.
By abusing select you are able to select a symbol FOO even
if FOO depends on BAR that is not set.
In general use select only for non-visible symbols
(no prompts anywhere) and for symbols with no dependencies.
That will limit the usefulness but on the other hand avoid
the illegal configurations all over.
BR,
Jani.
>
> Cc: Jani Nikula
> Cc: Joonas Lahtinen
> Cc: Rodrigo Vivi
> Cc: Tvrtko Ursulin
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Reported-by: Martin Liška
> Signed-off-by: Jiri Slaby (SUSE)
> ---
> drivers/gpu/drm/i915/display/intel_backlight.c | 2 +-
> drivers/gpu/drm/i915/display/intel_backlight.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c
> b/drivers/gpu/drm/i915/display/intel_backlight.c
> index beba39a38c87..c1ba68796b6d 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -825,7 +825,7 @@ void intel_backlight_enable(const struct intel_crtc_state
> *crtc_state,
> mutex_unlock(_priv->display.backlight.lock);
> }
>
> -#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> +#if IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)
> static u32 intel_panel_get_backlight(struct intel_connector *connector)
> {
> struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.h
> b/drivers/gpu/drm/i915/display/intel_backlight.h
> index 339643f63897..207fe1c613d8 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.h
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.h
> @@ -36,7 +36,7 @@ u32 intel_backlight_invert_pwm_level(struct intel_connector
> *connector, u32 leve
> u32 intel_backlight_level_to_pwm(struct intel_connector *connector, u32
> level);
> u32 intel_backlight_level_from_pwm(struct intel_connector *connector, u32
> val);
>
> -#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
> +#if IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)
> int intel_backlight_device_register(struct intel_connector *connector);
> void intel_backlight_device_unregister(struct intel_connector *connector);
> #else /* CONFIG_BACKLIGHT_CLASS_DEVICE */
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v1 17/17] drm/mediatek: Add mt8195-dpi support to drm_drv
On 03/10/2022 17:29, Guillaume Ranquet wrote:
> On Tue, 27 Sep 2022 16:28, Krzysztof Kozlowski
> wrote:
>> On 27/09/2022 15:04, Guillaume Ranquet wrote:
>>> On Thu, 22 Sep 2022 09:20, Krzysztof Kozlowski
>>> wrote:
On 19/09/2022 18:56, Guillaume Ranquet wrote:
> Add dpi support to enable the HDMI path.
>
> Signed-off-by: Guillaume Ranquet
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 72049a530ae1..27f029ca760b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -820,6 +820,8 @@ static const struct of_device_id
> mtk_ddp_comp_dt_ids[] = {
> .data = (void *)MTK_DPI },
> { .compatible = "mediatek,mt8192-dpi",
> .data = (void *)MTK_DPI },
> + { .compatible = "mediatek,mt8195-dpi",
> + .data = (void *)MTK_DPI },
It's compatible with the others. You don't need more compatibles.
>>>
>>> Hi Krzysztof,
>>>
>>> It's a bit confusing, because this compatible is used in both
>>> mtk_drm_drv.c and in mtk_dpi.c
>>>
>>> Albeit it's entirely the same thing regarding the mtk_drm_drv module,
>>> it's pretty different
>>> regarding the mtk_dpi module.
>>
>> Sure, but this does not explain why do you need these entries here in
>> mtk_drm_drv.
>>
>> Best regards,
>> Krzysztof
>>
>
> Hi Krzysztof,
>
> Sorry for the late answer.
> The mtk_drm_drv is the component master of the full mediatek drm stack.
>
> it "binds" all of the crtc/dpi/ovl/mutex/merge... components of the stack.
>
> That mtk_ddp_comp_dt_ids array is iterated over to find all of the components
> from the device tree.
No. You said what the code is doing. I think I understand this. You
still do not need more compatibles. Your sentence did not clarify it
because it did not answer at all to question "why". Why do you need it?
Sorry, the change looks not correct.
Best regards,
Krzysztof
[PATCH 2/2] drm/i915: remove circ_buf.h includes
The last user of macros from that include was removed in 2018 by the
commit below.
Fixes: 6cc42152b02b ("drm/i915: Remove support for legacy debugfs crc
interface")
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: David Airlie
Cc: Daniel Vetter
Cc: intel-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Jiri Slaby (SUSE)
---
drivers/gpu/drm/i915/display/intel_pipe_crc.c | 1 -
drivers/gpu/drm/i915/i915_irq.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_pipe_crc.c
b/drivers/gpu/drm/i915/display/intel_pipe_crc.c
index 8ac263f471be..9070935b0443 100644
--- a/drivers/gpu/drm/i915/display/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/display/intel_pipe_crc.c
@@ -24,7 +24,6 @@
*
*/
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 86a42d9e8041..09d728b34a47 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -28,7 +28,6 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include
#include
#include
--
2.37.3
[PATCH 1/2] drm/i915/display: fix randconfig build
When DRM_I915=y and BACKLIGHT_CLASS_DEVICE=m, the build fails:
ld: drivers/gpu/drm/i915/display/intel_backlight.o: in function
`intel_backlight_device_register':
intel_backlight.c:(.text+0x5587): undefined reference to
`backlight_device_get_by_name'
ld: drivers/gpu/drm/i915/display/intel_backlight.o: in function
`intel_backlight_device_unregister':
intel_backlight.c:(.text+0x576e): undefined reference to
`backlight_device_unregister'
To fix this, use IS_REACHABLE(), not IS_ENABLED() in backlight. That is,
with the above config, backlight support is disabled.
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: David Airlie
Cc: Daniel Vetter
Cc: intel-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Reported-by: Martin Liška
Signed-off-by: Jiri Slaby (SUSE)
---
drivers/gpu/drm/i915/display/intel_backlight.c | 2 +-
drivers/gpu/drm/i915/display/intel_backlight.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c
b/drivers/gpu/drm/i915/display/intel_backlight.c
index beba39a38c87..c1ba68796b6d 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -825,7 +825,7 @@ void intel_backlight_enable(const struct intel_crtc_state
*crtc_state,
mutex_unlock(_priv->display.backlight.lock);
}
-#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
+#if IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)
static u32 intel_panel_get_backlight(struct intel_connector *connector)
{
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.h
b/drivers/gpu/drm/i915/display/intel_backlight.h
index 339643f63897..207fe1c613d8 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.h
+++ b/drivers/gpu/drm/i915/display/intel_backlight.h
@@ -36,7 +36,7 @@ u32 intel_backlight_invert_pwm_level(struct intel_connector
*connector, u32 leve
u32 intel_backlight_level_to_pwm(struct intel_connector *connector, u32 level);
u32 intel_backlight_level_from_pwm(struct intel_connector *connector, u32 val);
-#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
+#if IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)
int intel_backlight_device_register(struct intel_connector *connector);
void intel_backlight_device_unregister(struct intel_connector *connector);
#else /* CONFIG_BACKLIGHT_CLASS_DEVICE */
--
2.37.3
Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/mtl: Add multicast steering for media GT
On 03/10/2022 20:32, Matt Roper wrote:
On Mon, Oct 03, 2022 at 09:56:18AM +0100, Tvrtko Ursulin wrote:
Hi Matt,
On 01/10/2022 01:45, Matt Roper wrote:
MTL's media GT only has a single type of steering ("OAADDRM") which
selects between media slice 0 and media slice 1. We'll always steer to
media slice 0 unless it is fused off (which is the case when VD0, VE0,
and SFC0 are all reported as unavailable).
Bspec: 67789
Signed-off-by: Matt Roper
---
drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 19 +--
drivers/gpu/drm/i915/gt/intel_gt_types.h| 1 +
drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 +-
3 files changed, 35 insertions(+), 3 deletions(-)
[snip]
+static void
+mtl_media_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
+{
+ /*
+* Unlike older platforms, we no longer setup implicit steering here;
+* all MCR accesses are explicitly steered.
+*/
+ if (drm_debug_enabled(DRM_UT_DRIVER)) {
+ struct drm_printer p = drm_debug_printer("MCR Steering:");
+
+ intel_gt_mcr_report_steering(, gt, false);
+ }
+}
+
static void
gt_init_workarounds(struct intel_gt *gt, struct i915_wa_list *wal)
{
struct drm_i915_private *i915 = gt->i915;
- if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
+ if (IS_METEORLAKE(i915) && gt->type == GT_MEDIA)
+ mtl_media_gt_workarounds_init(gt, wal);
+ else if (IS_METEORLAKE(i915) && gt->type == GT_PRIMARY)
mtl_3d_gt_workarounds_init(gt, wal);
else if (IS_PONTEVECCHIO(i915))
pvc_gt_workarounds_init(gt, wal);
Casually reading only - wouldn't it be nicer if the if-ladder in here
(gt_init_workarounds) would have a single case per platform, and then you'd
fork further (3d vs media) in MTL specific function?
Actually, that reminds me that we probably need to change this in a
different direction --- starting with MTL, we should stop tying
workarounds to the platform (IS_METEORLAKE) but rather tie them to the
IP version (i.e., GRAPHICS_VER or MEDIA_VER) since in the future the
same chiplets can potentially be re-used on multiple platforms.
Conversely, you could also potentially have variants of the same
"platform" (e.g., MTL) that incorporate chiplets with different IP
versions (and thus need distinct lists of workarounds and such).
At the moment MTL is the only platform we have with the standalone media
design so there's no potential for mix-and-match of chiplets yet, and
IS_METEORLAKE works fine in the short term, but we do need to start
planning ahead and moving off of platform checks in areas of the driver
like this.
Also, series ends up with mtl_media_gt_workarounds_init and
mtl_3d_gt_workarounds_init apparently 100% identical. You will need two
copies in the future?
Yes, the two GTs are expected to end up with completely different sets
of workarounds once the platform is enabled. We've just been delaying
on actually sending the new MTL workarounds upstream to give the
workaround database a bit more time to settle.
Ah yes, I misread the banner printed from those two "as no workaround
will be programmed from here" and thought why you'd need two copies of a
nearly empty function and two identical comments. My bad.
You will end up with three instances of "if debug report steering" so
could in theory add a helper for that. For some minimal value of
consolidation.. up to you.
Regards,
Tvrtko
linux-next: manual merge of the mm tree with the drm tree
Hi all,
Today's linux-next merge of the mm tree got a conflict in:
drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
between commit:
3a876060892b ("drm/amdkfd: Migrate in CPU page fault use current mm")
from the drm tree and commit:
c5f45c35acc4 ("mm/memory.c: fix race when faulting a device private page")
from the mm tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
index c70c026c9a93,97a684568ae0..
--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
@@@ -949,11 -940,11 +951,12 @@@ static vm_fault_t svm_migrate_to_ram(st
goto out_unlock_prange;
}
- r = svm_migrate_vram_to_ram(prange, mm,
KFD_MIGRATE_TRIGGER_PAGEFAULT_CPU,
- vmf->page);
+ r = svm_migrate_vram_to_ram(prange, vmf->vma->vm_mm,
- KFD_MIGRATE_TRIGGER_PAGEFAULT_CPU);
++ KFD_MIGRATE_TRIGGER_PAGEFAULT_CPU,
++ vmf->page);
if (r)
- pr_debug("failed %d migrate 0x%p [0x%lx 0x%lx] to ram\n", r,
- prange, prange->start, prange->last);
+ pr_debug("failed %d migrate svms 0x%p range 0x%p [0x%lx
0x%lx]\n",
+ r, prange->svms, prange, prange->start, prange->last);
/* xnack on, update mapping on GPUs with ACCESS_IN_PLACE */
if (p->xnack_enabled && parent == prange)
pgpC_ME3FNDyI.pgp
Description: OpenPGP digital signature
Re: [PATCH 0/5] drm: Fix math issues in MSM DSC implementation
On 2022-10-04 10:12:58, Vinod Koul wrote: > On 01-10-22, 21:08, Marijn Suijten wrote: > > Various removals of complex yet unnecessary math, fixing all uses of > > drm_dsc_config::bits_per_pixel to deal with the fact that this field > > includes four fractional bits, and finally an approach for dealing with > > dsi_host setting negative values in range_bpg_offset, resulting in > > overflow inside drm_dsc_pps_payload_pack(). > > > > Note that updating the static bpg_offset array to limit the size of > > these negative values to 6 bits changes what would be written to the DPU > > hardware at register(s) DSC_RANGE_BPG_OFFSET, hence the choice has been > > made to cover up for this while packing the value into a smaller field > > instead. > > Thanks for fixing these. I dont have my pixel3 availble but changes lgtm > > Reviewed-by: Vinod Koul Thanks; any comment on the self-review I sent in for patch 3 and 5? > > Altogether this series is responsible for solving _all_ Display Stream > > Compression issues and artifacts on the Sony Tama (sdm845) Akatsuki > > smartphone (2880x1440p). > > Does it need two dsi lanes? This panel has the default of four dsi data lanes enabled: https://github.com/sonyxperiadev/kernel/blob/f956fbd9a234033bd18234d456a2c32c126b38f3/arch/arm64/boot/dts/qcom/dsi-panel-somc-akatsuki.dtsi#L74-L77 Unless you are referring to dual-dsi (ctrl/phy); this panel doesn't have a dual connection, but I do have devices on sm8350/sm8450 with a "4k"@120Hz display that have this, in case you want it to be tested? However, for the time being I'm focussing on a similar panel (4 data lanes, single DSI ctrl/phy) on sm8250 which keeps showing corrupted / garbled data and resulting in ping-pong timeouts. I haven't yet confirmed if this is due to the "integration" of the pingpong block with the intf (since relevant registers and interrupts still seem to be accessible), a mismatching resource topology, or a misconfiguration elswhere. Relevant panel dts if you're interested: https://github.com/sonyxperiadev/kernel/blob/e70161ec43b147b0b02578d05ab64552fd2df2cd/arch/arm64/boot/dts/somc/dsi-panel-sofef03_m-fhd_plus.dtsi - Marijn
Re: [PATCH] drm/i915/pmu: Match frequencies reported by PMU and sysfs
On 03/10/2022 20:24, Ashutosh Dixit wrote:
PMU and sysfs use different wakeref's to "interpret" zero freq. Sysfs uses
runtime PM wakeref (see intel_rps_read_punit_req and
intel_rps_read_actual_frequency). PMU uses the GT parked/unparked
wakeref. In general the GT wakeref is held for less time that the runtime
PM wakeref which causes PMU to report a lower average freq than the average
freq obtained from sampling sysfs.
To resolve this, use the same freq functions (and wakeref's) in PMU as
those used in sysfs.
Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/7025
Reported-by: Ashwin Kumar Kulkarni
Cc: Tvrtko Ursulin
Signed-off-by: Ashutosh Dixit
---
drivers/gpu/drm/i915/i915_pmu.c | 27 ++-
1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 958b37123bf1..eda03f264792 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -371,37 +371,16 @@ static void
frequency_sample(struct intel_gt *gt, unsigned int period_ns)
{
struct drm_i915_private *i915 = gt->i915;
- struct intel_uncore *uncore = gt->uncore;
struct i915_pmu *pmu = >pmu;
struct intel_rps *rps = >rps;
if (!frequency_sampling_enabled(pmu))
return;
- /* Report 0/0 (actual/requested) frequency while parked. */
- if (!intel_gt_pm_get_if_awake(gt))
- return;
-
if (pmu->enable & config_mask(I915_PMU_ACTUAL_FREQUENCY)) {
- u32 val;
-
- /*
-* We take a quick peek here without using forcewake
-* so that we don't perturb the system under observation
-* (forcewake => !rc6 => increased power use). We expect
-* that if the read fails because it is outside of the
-* mmio power well, then it will return 0 -- in which
-* case we assume the system is running at the intended
-* frequency. Fortunately, the read should rarely fail!
-*/
- val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1);
- if (val)
- val = intel_rps_get_cagf(rps, val);
- else
- val = rps->cur_freq;
-
add_sample_mult(>sample[__I915_SAMPLE_FREQ_ACT],
- intel_gpu_freq(rps, val), period_ns / 1000);
+ intel_rps_read_actual_frequency(rps),
+ period_ns / 1000);
}
if (pmu->enable & config_mask(I915_PMU_REQUESTED_FREQUENCY)) {
What is software tracking of requested frequency showing when GT is
parked or runtime suspended? With this change sampling would be outside
any such checks so we need to be sure reported value makes sense.
Although more important open is around what is actually correct.
For instance how does the patch affect RC6 and power? I don't know how
power management of different blocks is wired up, so personally I would
only be able to look at it empirically. In other words what I am asking
is this - if we changed from skipping obtaining forcewake even when
unparked, to obtaining forcewake if not runtime suspended - what
hardware blocks does that power up and how it affects RC6 and power? Can
it affect actual frequency or not? (Will "something" power up the clocks
just because we will be getting forcewake?)
Or maybe question simplified - does 200Hz polling on existing sysfs
actual frequency field disturbs the system under some circumstances?
(Increases power and decreases RC6.) If it does then that would be a
problem. We want a solution which shows the real data, but where the act
of monitoring itself does not change it too much. If it doesn't then
it's okay.
Could you somehow investigate on these topics? Maybe log RAPL GPU power
while polling on sysfs, versus getting the actual frequency from the
existing PMU implementation and see if that shows anything? Or actually
simpler - RAPL GPU power for current PMU intel_gpu_top versus this
patch? On idle(-ish) desktop workloads perhaps? Power and frequency
graphed for both.
Regards,
Tvrtko
@@ -409,8 +388,6 @@ frequency_sample(struct intel_gt *gt, unsigned int
period_ns)
intel_rps_get_requested_frequency(rps),
period_ns / 1000);
}
-
- intel_gt_pm_put_async(gt);
}
static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
Re: [RFC/PATCH] backlight: hx8357: prepare to conversion to gpiod API
On Wed, Sep 28, 2022 at 12:32 AM Dmitry Torokhov wrote: > Properties describing GPIOs should be named as "-gpios" or > "-gpio", and that is what gpiod API expects, however the > driver uses non-standard "gpios-reset" name. Let's adjust this, and also > note that the reset line is active low as that is also important to > gpiod API. > > Signed-off-by: Dmitry Torokhov I think the gods of Open Firmware will try to punish you for such incompatible changes. But I have long since renounced them. > Another option is to add another quirk into gpiolib-of.c, but we > may end up with a ton of them once we convert everything away from > of_get_named_gpio() to gpiod API, so I'd prefer not doing that. We need to know if i.MX is shipping device trees stored in flash, or if they bundle it with the kernel. In the former case, you have to add quirks, in the latter case this patch is fine. Sascha, what does the Freescale maintainer say? Yours, Linus Walleij
Re: [PATCH v3 0/2] drm/bridge: it6505: Power management fixes for it6505 bridge
On Tue, 4 Oct 2022 at 06:49, Pin-yen Lin wrote: > > This series contains 2 fixes related to it6505 power management. > > Changes in v3: > - Handle the error from extcon_get_state > - Collect review tag > > Changes in v2: > - Handle the error from pm_runtime_get_sync in it6505_extcon_work > > Pin-yen Lin (2): > drm/bridge: it6505: Adapt runtime power management framework > drm/bridge: it6505: Add pre_enable/post_disable callback > > drivers/gpu/drm/bridge/ite-it6505.c | 58 - > 1 file changed, 48 insertions(+), 10 deletions(-) > > -- > 2.38.0.rc1.362.ged0d419d3c-goog > Applied to drm-misc-next.
Re: [PATCH v3 1/2] drm/bridge: it6505: Adapt runtime power management framework
Il 04/10/22 06:49, Pin-yen Lin ha scritto:
Use pm_runtime_(get|put)_sync to control the bridge power, and add
SET_SYSTEM_SLEEP_PM_OPS with pm_runtime_force_(suspend|resume) to it6505
driver. Without SET_SYSTEM_SLEEP_PM_OPS, the bridge will be powered on
unnecessarily when no external display is connected.
Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Pin-yen Lin
Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH 1/2] drm/i915: Add a wrapper for frequency debugfs
On Mon, 03 Oct 2022, Vinay Belgaumkar wrote:
> Move it to the RPS source file.
The idea was that the 1st patch would be non-functional code
movement. This is still a functional change.
Or you can do the functional changes first, and then move code, as long
as you don't combine code movement with functional changes.
Please also mark your patch revisions and note the changes. There's no
indication this series is v2.
BR,
Jani.
>
> Signed-off-by: Vinay Belgaumkar
> ---
> drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 157 +---
> drivers/gpu/drm/i915/gt/intel_rps.c | 169 ++
> drivers/gpu/drm/i915/gt/intel_rps.h | 3 +
> 3 files changed, 173 insertions(+), 156 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> index 9fd4d9255a97..4319d6cdafe2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c
> @@ -344,162 +344,7 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt,
> struct drm_printer *p)
> drm_printf(p, "efficient (RPe) frequency: %d MHz\n",
> intel_gpu_freq(rps, rps->efficient_freq));
> } else if (GRAPHICS_VER(i915) >= 6) {
> - u32 rp_state_limits;
> - u32 gt_perf_status;
> - struct intel_rps_freq_caps caps;
> - u32 rpmodectl, rpinclimit, rpdeclimit;
> - u32 rpstat, cagf, reqf;
> - u32 rpcurupei, rpcurup, rpprevup;
> - u32 rpcurdownei, rpcurdown, rpprevdown;
> - u32 rpupei, rpupt, rpdownei, rpdownt;
> - u32 pm_ier, pm_imr, pm_isr, pm_iir, pm_mask;
> -
> - rp_state_limits = intel_uncore_read(uncore,
> GEN6_RP_STATE_LIMITS);
> - gen6_rps_get_freq_caps(rps, );
> - if (IS_GEN9_LP(i915))
> - gt_perf_status = intel_uncore_read(uncore,
> BXT_GT_PERF_STATUS);
> - else
> - gt_perf_status = intel_uncore_read(uncore,
> GEN6_GT_PERF_STATUS);
> -
> - /* RPSTAT1 is in the GT power well */
> - intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
> -
> - reqf = intel_uncore_read(uncore, GEN6_RPNSWREQ);
> - if (GRAPHICS_VER(i915) >= 9) {
> - reqf >>= 23;
> - } else {
> - reqf &= ~GEN6_TURBO_DISABLE;
> - if (IS_HASWELL(i915) || IS_BROADWELL(i915))
> - reqf >>= 24;
> - else
> - reqf >>= 25;
> - }
> - reqf = intel_gpu_freq(rps, reqf);
> -
> - rpmodectl = intel_uncore_read(uncore, GEN6_RP_CONTROL);
> - rpinclimit = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
> - rpdeclimit = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
> -
> - rpstat = intel_uncore_read(uncore, GEN6_RPSTAT1);
> - rpcurupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) &
> GEN6_CURICONT_MASK;
> - rpcurup = intel_uncore_read(uncore, GEN6_RP_CUR_UP) &
> GEN6_CURBSYTAVG_MASK;
> - rpprevup = intel_uncore_read(uncore, GEN6_RP_PREV_UP) &
> GEN6_CURBSYTAVG_MASK;
> - rpcurdownei = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN_EI) &
> GEN6_CURIAVG_MASK;
> - rpcurdown = intel_uncore_read(uncore, GEN6_RP_CUR_DOWN) &
> GEN6_CURBSYTAVG_MASK;
> - rpprevdown = intel_uncore_read(uncore, GEN6_RP_PREV_DOWN) &
> GEN6_CURBSYTAVG_MASK;
> -
> - rpupei = intel_uncore_read(uncore, GEN6_RP_UP_EI);
> - rpupt = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD);
> -
> - rpdownei = intel_uncore_read(uncore, GEN6_RP_DOWN_EI);
> - rpdownt = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD);
> -
> - cagf = intel_rps_read_actual_frequency(rps);
> -
> - intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
> -
> - if (GRAPHICS_VER(i915) >= 11) {
> - pm_ier = intel_uncore_read(uncore,
> GEN11_GPM_WGBOXPERF_INTR_ENABLE);
> - pm_imr = intel_uncore_read(uncore,
> GEN11_GPM_WGBOXPERF_INTR_MASK);
> - /*
> - * The equivalent to the PM ISR & IIR cannot be read
> - * without affecting the current state of the system
> - */
> - pm_isr = 0;
> - pm_iir = 0;
> - } else if (GRAPHICS_VER(i915) >= 8) {
> - pm_ier = intel_uncore_read(uncore, GEN8_GT_IER(2));
> - pm_imr = intel_uncore_read(uncore, GEN8_GT_IMR(2));
> - pm_isr = intel_uncore_read(uncore, GEN8_GT_ISR(2));
> - pm_iir = intel_uncore_read(uncore, GEN8_GT_IIR(2));
> - } else {
> - pm_ier =
Re: linux-next: manual merge of the fbdev tree with the drm tree
On 10/4/22 04:27, Stephen Rothwell wrote:
Hi all,
Today's linux-next merge of the fbdev tree got a conflict in:
drivers/video/fbdev/tridentfb.c
between commit:
145eed48de27 ("fbdev: Remove conflicting devices on PCI bus")
from the drm tree and commit:
d738bf0123d6 ("fbdev: tridentfb: Fix missing pci_disable_device() in probe and
remove")
from the fbdev tree.
Thanks for the notice!
I'll fix it up in the fbdev tree, either by dropping that offending patch or
fix the conflict.
Helge
[PATCH] drm/display: Drop obsolete dependency on COMPILE_TEST
Since commit 0166dc11be91 ("of: make CONFIG_OF user selectable"), it
is possible to test-build any driver which depends on OF on any
architecture by explicitly selecting OF. Therefore depending on
COMPILE_TEST as an alternative is no longer needed.
It is actually better to always build such drivers with OF enabled,
so that the test builds are closer to how each driver will actually be
built on its intended target. Building them without OF may not test
much as the compiler will optimize out potentially large parts of the
code. In the worst case, this could even pop false positive warnings.
Dropping COMPILE_TEST here improves the quality of our testing and
avoids wasting time on non-existent issues.
Signed-off-by: Jean Delvare
Cc: David Airlie
Cc: Daniel Vetter
---
drivers/gpu/drm/display/Kconfig |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-5.19.orig/drivers/gpu/drm/display/Kconfig
+++ linux-5.19/drivers/gpu/drm/display/Kconfig
@@ -3,7 +3,7 @@
config DRM_DP_AUX_BUS
tristate
depends on DRM
- depends on OF || COMPILE_TEST
+ depends on OF
config DRM_DISPLAY_HELPER
tristate
--
Jean Delvare
SUSE L3 Support
