Re: [Freedreno] [PATCH 3/5] drm/msm: Fix IS_ERR_OR_NULL() vs NULL check in msm_icc_get()
NAK. This path should be returned if it is NON-NULL, otherwise we defer
to of_icc_get() on the parent device. See the code-comment below.
Thanks for taking time to review this patch, how do you think of the following
changes:
diff --git a/drivers/gpu/drm/msm/msm_io_utils.c
b/drivers/gpu/drm/msm/msm_io_utils.c index d02cd29ce829..a112d8c74d59
100644 --- a/drivers/gpu/drm/msm/msm_io_utils.c +++
b/drivers/gpu/drm/msm/msm_io_utils.c @@ -133,7 +133,7 @@ struct
icc_path *msm_icc_get(struct device *dev, const char *name) struct
icc_path *path; path = of_icc_get(dev, name); - if (path) + if
(!IS_ERR_OR_NULL(path)) return path;
Looking forward to your reply, thanks again!
On 2022/11/11 18:02, Marijn Suijten wrote:
On 2022-11-10 17:44:43, Gaosheng Cui wrote:
The of_icc_get() function returns NULL or error pointers on error path,
so we should use IS_ERR_OR_NULL() to check the return value.
Fixes: 5ccdcecaf8f7 ("drm/msm: lookup the ICC paths in both mdp5/dpu and mdss
devices")
Signed-off-by: Gaosheng Cui
---
drivers/gpu/drm/msm/msm_io_utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_io_utils.c
b/drivers/gpu/drm/msm/msm_io_utils.c
index d02cd29ce829..950083b2f092 100644
--- a/drivers/gpu/drm/msm/msm_io_utils.c
+++ b/drivers/gpu/drm/msm/msm_io_utils.c
@@ -133,7 +133,7 @@ struct icc_path *msm_icc_get(struct device *dev, const char
*name)
struct icc_path *path;
path = of_icc_get(dev, name);
- if (path)
+ if (IS_ERR_OR_NULL(path))
NAK. This path should be returned if it is NON-NULL, otherwise we defer
to of_icc_get() on the parent device. See the code-comment below.
- Marijn
return path;
/*
--
2.25.1
.
Re: [Freedreno] [v1] drm/msm/disp/dpu1: add color management support for the crtc
On 11/11/2022 16:57, Kalyan Thota wrote:
Add color management support for the crtc provided there are
enough dspps that can be allocated from the catalogue.
Changes in v1:
- cache color enabled state in the dpu crtc obj (Dmitry)
- simplify dspp allocation while creating crtc (Dmitry)
- register for color when crtc is created (Dmitry)
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 5 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h| 6 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 53 -
4 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 4170fbe..ca4c3b3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1571,7 +1571,7 @@ static const struct drm_crtc_helper_funcs
dpu_crtc_helper_funcs = {
/* initialize crtc */
struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane
*plane,
- struct drm_plane *cursor)
+ struct drm_plane *cursor, unsigned long
features)
{
struct drm_crtc *crtc = NULL;
struct dpu_crtc *dpu_crtc = NULL;
@@ -1583,6 +1583,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev,
struct drm_plane *plane,
crtc = _crtc->base;
crtc->dev = dev;
+ dpu_crtc->color_enabled = features & BIT(DPU_DSPP_PCC);
spin_lock_init(_crtc->spin_lock);
atomic_set(_crtc->frame_pending, 0);
@@ -1604,7 +1605,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev,
struct drm_plane *plane,
drm_crtc_helper_add(crtc, _crtc_helper_funcs);
- drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
+ drm_crtc_enable_color_mgmt(crtc, 0, dpu_crtc->color_enabled, 0);
/* save user friendly CRTC name for later */
snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u", crtc->base.id);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 539b68b..342f9ae 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -136,6 +136,7 @@ struct dpu_crtc_frame_event {
* @enabled : whether the DPU CRTC is currently enabled. updated in the
* commit-thread, not state-swap time which is earlier, so
* safe to make decisions on during VBLANK on/off work
+ * @color_enabled : whether crtc supports color management
* @feature_list : list of color processing features supported on a crtc
* @active_list : list of color processing features are active
* @dirty_list: list of color processing features are dirty
@@ -164,7 +165,7 @@ struct dpu_crtc {
u64 play_count;
ktime_t vblank_cb_time;
bool enabled;
-
+ bool color_enabled;
struct list_head feature_list;
struct list_head active_list;
struct list_head dirty_list;
@@ -269,10 +270,11 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc);
* @dev: dpu device
* @plane: base plane
* @cursor: cursor plane
+ * @features: color features
* @Return: new crtc object or error
*/
struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane
*plane,
- struct drm_plane *cursor);
+ struct drm_plane *cursor, unsigned long
features);
/**
* dpu_crtc_register_custom_event - api for enabling/disabling crtc event
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index c9058aa..d72edb8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -579,6 +579,7 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
static struct msm_display_topology dpu_encoder_get_topology(
struct dpu_encoder_virt *dpu_enc,
struct dpu_kms *dpu_kms,
+ struct dpu_crtc *dpu_crtc,
struct drm_display_mode *mode)
{
struct msm_display_topology topology = {0};
@@ -607,7 +608,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
else
topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
- if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
+ if (dpu_crtc->color_enabled) {
if (dpu_kms->catalog->dspp &&
(dpu_kms->catalog->dspp_count >= topology.num_lm))
topology.num_dspp = topology.num_lm;
@@ -642,6 +643,7 @@ static int dpu_encoder_virt_atomic_check(
struct drm_display_mode *adj_mode;
struct msm_display_topology topology;
struct dpu_global_state *global_state;
+ struct dpu_crtc *dpu_crtc;
int i = 0;
int ret = 0;
@@
Re: [Freedreno] [v1] drm/msm/disp/dpu1: populate disp_info with connector type
On 11/11/2022 16:56, Kalyan Thota wrote:
Populate disp_info with connector type. Since DRM encoder type
for few encoders can be similar (like eDP and DP) this information
will be useful to differentiate interfaces.
Changes in v1:
- add connector type in the disp_info (Dmitry)
You can get connector type from
- add helper functions to know encoder type
- update commit text reflecting the change
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 26 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 ++
drivers/gpu/drm/msm/dp/dp_display.c | 5
drivers/gpu/drm/msm/msm_drv.h | 7 -
5 files changed, 77 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b..c9058aa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -217,6 +217,40 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
};
+bool dpu_encoder_is_external(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+
+ if (!drm_enc)
+ return false;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ return (dpu_enc->disp_info.connector_type ==
+ DRM_MODE_CONNECTOR_DisplayPort);
And also HDMI, DVI, VGA and several other connector types.
It is much easier to enumerate non-interesting (in other words,
non-external ones):
- Unknown
- LVDS
- eDP
- DSI
- DPI
- VIRTUAL
- WRITEBACK
+}
+
+bool dpu_encoder_is_virtual(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+
+ if (!drm_enc)
+ return false;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ return (dpu_enc->disp_info.connector_type ==
DRM_MODE_CONNECTOR_WRITEBACK);
+}
+
+bool dpu_encoder_is_primary(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+
+ if (!drm_enc)
+ return false;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ return((dpu_enc->disp_info.connector_type == DRM_MODE_CONNECTOR_DSI) ||
+ (dpu_enc->disp_info.connector_type == DRM_MODE_CONNECTOR_eDP));
Why do you need a separate is_primary?
+}
bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
{
@@ -2412,7 +2446,7 @@ int dpu_encoder_setup(struct drm_device *dev, struct
drm_encoder *enc,
struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
struct drm_encoder *drm_enc = NULL;
struct dpu_encoder_virt *dpu_enc = NULL;
- int ret = 0;
+ int ret = 0, intf_i;
dpu_enc = to_dpu_encoder_virt(enc);
@@ -2424,13 +2458,17 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
timer_setup(_enc->frame_done_timer,
dpu_encoder_frame_done_timeout, 0);
+ intf_i = disp_info->h_tile_instance[0];
if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
timer_setup(_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
0);
- else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+ else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
- priv->dp[disp_info->h_tile_instance[0]]);
+ priv->dp[intf_i]);
+ disp_info->connector_type =
+ msm_dp_get_connector_type(priv->dp[intf_i]);
+ }
INIT_DELAYED_WORK(_enc->delayed_off_work,
dpu_encoder_off_work);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236e..d361c5d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -25,16 +25,18 @@
* @num_of_h_tiles: Number of horizontal tiles in case of split interface
* @h_tile_instance:Controller instance used per tile. Number of elements
is
* based on num_of_h_tiles
- * @is_cmd_modeBoolean to indicate if the CMD mode is requested
+ * @is_cmd_mode:Boolean to indicate if the CMD mode is requested
Unrelated change. If you want to fix a whitespace, please do so. In a
separate patch.
+ * @connector_type: DRM_MODE_CONNECTOR_ type
You can get this kind of information from the atomic state.
See the for_each_connector_on_encoder
* @is_te_using_watchdog_timer: Boolean to indicate watchdog TE is
- * used instead of panel TE in cmd mode panels
- * @dsc: DSC configuration data for DSC-enabled displays
+ * used instead of panel TE in cmd mode panels
+ *
Re: [Freedreno] [v1] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder
On 11/11/2022 16:56, Kalyan Thota wrote:
Pin each crtc with one encoder. This arrangement will
disallow crtc switching between encoders and also will
facilitate to advertise certain features on crtc based
on encoder type.
Changes in v1:
- use drm_for_each_encoder macro while iterating through
encoder list (Dmitry)
BTW: if these patches form a series, please send them so.
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7a5fabc..0d94eec0d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -798,19 +798,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
max_crtc_count = min(max_crtc_count, primary_planes_idx);
/* Create one CRTC per encoder */
- for (i = 0; i < max_crtc_count; i++) {
- crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
- if (IS_ERR(crtc)) {
- ret = PTR_ERR(crtc);
- return ret;
+ i = 0;
+ drm_for_each_encoder(encoder, dev) {
+ if (i < max_crtc_count) {
What if max_crtc_counter < num_encoders? I think we should disallow such
configuration. Can it happen on any of relevant platforms?
+ crtc = dpu_crtc_init(dev, primary_planes[i],
cursor_planes[i]);
+ if (IS_ERR(crtc)) {
+ ret = PTR_ERR(crtc);
+ return ret;
+ }
+ priv->crtcs[priv->num_crtcs++] = crtc;
+ encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
}
- priv->crtcs[priv->num_crtcs++] = crtc;
+ i++;
}
- /* All CRTCs are compatible with all encoders */
- drm_for_each_encoder(encoder, dev)
- encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
-
return 0;
}
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 2/2] adreno: Detect shutdown during get_param()
On Fri, Nov 11, 2022 at 4:37 PM Joel Fernandes
wrote:
>
>
> > On Nov 11, 2022, at 4:28 PM, Akhil P Oommen
> wrote:
> >
> > On 11/12/2022 1:19 AM, Joel Fernandes (Google) wrote:
> >> Even though the GPU is shut down, during kexec reboot we can have
> userspace
> >> still running. This is especially true if KEXEC_JUMP is not enabled,
> because we
> >> do not freeze userspace in this case.
> >>
> >> To prevent crashes, track that the GPU is shutdown and prevent
> get_param() from
> >> accessing GPU resources if we find it shutdown.
> >>
> >> This fixes the following crash during kexec reboot on an ARM64 device
> with adreno GPU:
> >>
> >> [ 292.534314] Kernel panic - not syncing: Asynchronous SError Interrupt
> >> [ 292.534323] Hardware name: Google Lazor (rev3 - 8) with LTE (DT)
> >> [ 292.534326] Call trace:
> >> [ 292.534328] dump_backtrace+0x0/0x1d4
> >> [ 292.534337] show_stack+0x20/0x2c
> >> [ 292.534342] dump_stack_lvl+0x60/0x78
> >> [ 292.534347] dump_stack+0x18/0x38
> >> [ 292.534352] panic+0x148/0x3b0
> >> [ 292.534357] nmi_panic+0x80/0x94
> >> [ 292.534364] arm64_serror_panic+0x70/0x7c
> >> [ 292.534369] do_serror+0x0/0x7c
> >> [ 292.534372] do_serror+0x54/0x7c
> >> [ 292.534377] el1h_64_error_handler+0x34/0x4c
> >> [ 292.534381] el1h_64_error+0x7c/0x80
> >> [ 292.534386] el1_interrupt+0x20/0x58
> >> [ 292.534389] el1h_64_irq_handler+0x18/0x24
> >> [ 292.534395] el1h_64_irq+0x7c/0x80
> >> [ 292.534399] local_daif_inherit+0x10/0x18
> >> [ 292.534405] el1h_64_sync_handler+0x48/0xb4
> >> [ 292.534410] el1h_64_sync+0x7c/0x80
> >> [ 292.534414] a6xx_gmu_set_oob+0xbc/0x1fc
> >> [ 292.534422] a6xx_get_timestamp+0x40/0xb4
> >> [ 292.534426] adreno_get_param+0x12c/0x1e0
> >> [ 292.534433] msm_ioctl_get_param+0x64/0x70
> >> [ 292.534440] drm_ioctl_kernel+0xe8/0x158
> >> [ 292.534448] drm_ioctl+0x208/0x320
> >> [ 292.534453] __arm64_sys_ioctl+0x98/0xd0
> >> [ 292.534461] invoke_syscall+0x4c/0x118
> >> [ 292.534467] el0_svc_common+0x98/0x104
> >> [ 292.534473] do_el0_svc+0x30/0x80
> >> [ 292.534478] el0_svc+0x20/0x50
> >> [ 292.534481] el0t_64_sync_handler+0x78/0x108
> >> [ 292.534485] el0t_64_sync+0x1a4/0x1a8
> >> [ 292.534632] Kernel Offset: 0x1a5f80 from 0xffc00800
> >> [ 292.534635] PHYS_OFFSET: 0x8000
> >> [ 292.534638] CPU features: 0x40018541,a3300e42
> >> [ 292.534644] Memory Limit: none
> >>
> >> Cc: Rob Clark
> >> Cc: Steven Rostedt
> >> Cc: Ricardo Ribalda
> >> Cc: Ross Zwisler
> >> Signed-off-by: Joel Fernandes (Google)
> >> ---
> >> drivers/gpu/drm/msm/adreno/adreno_device.c | 1 +
> >> drivers/gpu/drm/msm/adreno/adreno_gpu.c| 2 +-
> >> drivers/gpu/drm/msm/msm_gpu.h | 3 +++
> >> 3 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> index f0cff62812c3..03d912dc0130 100644
> >> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> >> @@ -612,6 +612,7 @@ static void adreno_shutdown(struct platform_device
> *pdev)
> >> {
> >> struct msm_gpu *gpu = dev_to_gpu(>dev);
> >> +gpu->is_shutdown = true;
> >> WARN_ON_ONCE(adreno_system_suspend(>dev));
> >> }
> >> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> >> index 382fb7f9e497..6903c6892469 100644
> >> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> >> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> >> @@ -251,7 +251,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct
> msm_file_private *ctx,
> >> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> >>/* No pointer params yet */
> >> -if (*len != 0)
> >> +if (*len != 0 || gpu->is_shutdown)
> >> return -EINVAL;
> > This will race with shutdown.
>
> Could you clarify what you mean? At this point in the code, the shutdown
> is completed and it crashes here.
>
Ok so I think you meant that if the shut down happens after we sample the
is_shutdown, then we run into the same issue.
I can’t reproduce that but I’ll look into that. Another way might be to
synchronize using a mutex. Though maybe the shutdown path can wait for
active pm_runtime references?
Thanks.
> > Probably, propagating back the return value of pm_runtime_get() in every
> possible ioctl call path is the right thing to do.
>
> Ok I’ll look into that. But the patch I posted works reliably and fixes
> all crashes we could reproduce.
>
> > I have never thought about this scenario. Do you know why userspace is
> not freezed before kexec?
>
> I am not sure. It depends on how kexec is used. The userspace freeze
> happens only when kexec is called to switch back and forth between
> different kernels (persistence mode). In such scenario I believe the
> userspace has to be frozen and unfrozen. However for normal kexec, that
> does not happen.
>
> Thanks.
>
>
> >
> > -Akhil.
> >>
Re: [Freedreno] [v9] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280
On 11/11/2022 16:55, 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)
Changes in v7:
- Cache flush mask per dspp (Dmitry)
- Few nits (Marijn)
Changes in v8:
- Few nits (Marijn)
Changes in v9:
- use DSPP enum while accessing flush mask to make it readable (Dmitry)
- Few nits (Dmitry)
Signed-off-by: Kalyan Thota
---
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 | 64 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 5 +-
5 files changed, 65 insertions(+), 15 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..126ee37 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..0ee8220 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -28,22 +28,23 @@
#define CTL_INTF_ACTIVE 0x0F4
#define CTL_MERGE_3D_FLUSH0x100
#define CTL_DSC_ACTIVE0x0E8
-#define CTL_DSC_FLUSH0x104
+#define CTL_DSC_FLUSH 0x104
#define CTL_WB_FLUSH 0x108
#define CTL_INTF_FLUSH0x110
#define CTL_INTF_MASTER 0x134
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
+#define CTL_DSPP_n_FLUSH(n) ((0x13C) + ((n) * 4))
-#define CTL_MIXER_BORDER_OUTBIT(24)
-#define CTL_FLUSH_MASK_CTL BIT(17)
+#define CTL_MIXER_BORDER_OUT BIT(24)
+#define CTL_FLUSH_MASK_CTLBIT(17)
Whitespace changes should go to a separate patch. And I'd prefer to have
extra whitespaces removed, not added.
Other than that LGTM now.
-#define DPU_REG_RESET_TIMEOUT_US2000
-#define MERGE_3D_IDX 23
-#define DSC_IDX22
-#define INTF_IDX 31
-#define WB_IDX 16
-#define CTL_INVALID_BIT 0x
-#define CTL_DEFAULT_GROUP_ID 0xf
+#define
Re: [Freedreno] [PATCH 2/2] adreno: Detect shutdown during get_param()
> On Nov 11, 2022, at 4:28 PM, Akhil P Oommen wrote:
>
> On 11/12/2022 1:19 AM, Joel Fernandes (Google) wrote:
>> Even though the GPU is shut down, during kexec reboot we can have userspace
>> still running. This is especially true if KEXEC_JUMP is not enabled, because
>> we
>> do not freeze userspace in this case.
>>
>> To prevent crashes, track that the GPU is shutdown and prevent get_param()
>> from
>> accessing GPU resources if we find it shutdown.
>>
>> This fixes the following crash during kexec reboot on an ARM64 device with
>> adreno GPU:
>>
>> [ 292.534314] Kernel panic - not syncing: Asynchronous SError Interrupt
>> [ 292.534323] Hardware name: Google Lazor (rev3 - 8) with LTE (DT)
>> [ 292.534326] Call trace:
>> [ 292.534328] dump_backtrace+0x0/0x1d4
>> [ 292.534337] show_stack+0x20/0x2c
>> [ 292.534342] dump_stack_lvl+0x60/0x78
>> [ 292.534347] dump_stack+0x18/0x38
>> [ 292.534352] panic+0x148/0x3b0
>> [ 292.534357] nmi_panic+0x80/0x94
>> [ 292.534364] arm64_serror_panic+0x70/0x7c
>> [ 292.534369] do_serror+0x0/0x7c
>> [ 292.534372] do_serror+0x54/0x7c
>> [ 292.534377] el1h_64_error_handler+0x34/0x4c
>> [ 292.534381] el1h_64_error+0x7c/0x80
>> [ 292.534386] el1_interrupt+0x20/0x58
>> [ 292.534389] el1h_64_irq_handler+0x18/0x24
>> [ 292.534395] el1h_64_irq+0x7c/0x80
>> [ 292.534399] local_daif_inherit+0x10/0x18
>> [ 292.534405] el1h_64_sync_handler+0x48/0xb4
>> [ 292.534410] el1h_64_sync+0x7c/0x80
>> [ 292.534414] a6xx_gmu_set_oob+0xbc/0x1fc
>> [ 292.534422] a6xx_get_timestamp+0x40/0xb4
>> [ 292.534426] adreno_get_param+0x12c/0x1e0
>> [ 292.534433] msm_ioctl_get_param+0x64/0x70
>> [ 292.534440] drm_ioctl_kernel+0xe8/0x158
>> [ 292.534448] drm_ioctl+0x208/0x320
>> [ 292.534453] __arm64_sys_ioctl+0x98/0xd0
>> [ 292.534461] invoke_syscall+0x4c/0x118
>> [ 292.534467] el0_svc_common+0x98/0x104
>> [ 292.534473] do_el0_svc+0x30/0x80
>> [ 292.534478] el0_svc+0x20/0x50
>> [ 292.534481] el0t_64_sync_handler+0x78/0x108
>> [ 292.534485] el0t_64_sync+0x1a4/0x1a8
>> [ 292.534632] Kernel Offset: 0x1a5f80 from 0xffc00800
>> [ 292.534635] PHYS_OFFSET: 0x8000
>> [ 292.534638] CPU features: 0x40018541,a3300e42
>> [ 292.534644] Memory Limit: none
>>
>> Cc: Rob Clark
>> Cc: Steven Rostedt
>> Cc: Ricardo Ribalda
>> Cc: Ross Zwisler
>> Signed-off-by: Joel Fernandes (Google)
>> ---
>> drivers/gpu/drm/msm/adreno/adreno_device.c | 1 +
>> drivers/gpu/drm/msm/adreno/adreno_gpu.c| 2 +-
>> drivers/gpu/drm/msm/msm_gpu.h | 3 +++
>> 3 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
>> b/drivers/gpu/drm/msm/adreno/adreno_device.c
>> index f0cff62812c3..03d912dc0130 100644
>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
>> @@ -612,6 +612,7 @@ static void adreno_shutdown(struct platform_device *pdev)
>> {
>> struct msm_gpu *gpu = dev_to_gpu(>dev);
>> +gpu->is_shutdown = true;
>> WARN_ON_ONCE(adreno_system_suspend(>dev));
>> }
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> index 382fb7f9e497..6903c6892469 100644
>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> @@ -251,7 +251,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct
>> msm_file_private *ctx,
>> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>/* No pointer params yet */
>> -if (*len != 0)
>> +if (*len != 0 || gpu->is_shutdown)
>> return -EINVAL;
> This will race with shutdown.
Could you clarify what you mean? At this point in the code, the shutdown is
completed and it crashes here.
> Probably, propagating back the return value of pm_runtime_get() in every
> possible ioctl call path is the right thing to do.
Ok I’ll look into that. But the patch I posted works reliably and fixes all
crashes we could reproduce.
> I have never thought about this scenario. Do you know why userspace is not
> freezed before kexec?
I am not sure. It depends on how kexec is used. The userspace freeze happens
only when kexec is called to switch back and forth between different kernels
(persistence mode). In such scenario I believe the userspace has to be frozen
and unfrozen. However for normal kexec, that does not happen.
Thanks.
>
> -Akhil.
>>switch (param) {
>> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
>> index ff911e7305ce..f18b0a91442b 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu.h
>> +++ b/drivers/gpu/drm/msm/msm_gpu.h
>> @@ -214,6 +214,9 @@ struct msm_gpu {
>> /* does gpu need hw_init? */
>> bool needs_hw_init;
>> +/* is the GPU shutdown? */
>> +bool is_shutdown;
>> +
>> /**
>> * global_faults: number of GPU hangs not attributed to a particular
>> * address space
>
Re: [Freedreno] [PATCH 2/2] adreno: Detect shutdown during get_param()
On 11/12/2022 1:19 AM, Joel Fernandes (Google) wrote:
Even though the GPU is shut down, during kexec reboot we can have userspace
still running. This is especially true if KEXEC_JUMP is not enabled, because we
do not freeze userspace in this case.
To prevent crashes, track that the GPU is shutdown and prevent get_param() from
accessing GPU resources if we find it shutdown.
This fixes the following crash during kexec reboot on an ARM64 device with
adreno GPU:
[ 292.534314] Kernel panic - not syncing: Asynchronous SError Interrupt
[ 292.534323] Hardware name: Google Lazor (rev3 - 8) with LTE (DT)
[ 292.534326] Call trace:
[ 292.534328] dump_backtrace+0x0/0x1d4
[ 292.534337] show_stack+0x20/0x2c
[ 292.534342] dump_stack_lvl+0x60/0x78
[ 292.534347] dump_stack+0x18/0x38
[ 292.534352] panic+0x148/0x3b0
[ 292.534357] nmi_panic+0x80/0x94
[ 292.534364] arm64_serror_panic+0x70/0x7c
[ 292.534369] do_serror+0x0/0x7c
[ 292.534372] do_serror+0x54/0x7c
[ 292.534377] el1h_64_error_handler+0x34/0x4c
[ 292.534381] el1h_64_error+0x7c/0x80
[ 292.534386] el1_interrupt+0x20/0x58
[ 292.534389] el1h_64_irq_handler+0x18/0x24
[ 292.534395] el1h_64_irq+0x7c/0x80
[ 292.534399] local_daif_inherit+0x10/0x18
[ 292.534405] el1h_64_sync_handler+0x48/0xb4
[ 292.534410] el1h_64_sync+0x7c/0x80
[ 292.534414] a6xx_gmu_set_oob+0xbc/0x1fc
[ 292.534422] a6xx_get_timestamp+0x40/0xb4
[ 292.534426] adreno_get_param+0x12c/0x1e0
[ 292.534433] msm_ioctl_get_param+0x64/0x70
[ 292.534440] drm_ioctl_kernel+0xe8/0x158
[ 292.534448] drm_ioctl+0x208/0x320
[ 292.534453] __arm64_sys_ioctl+0x98/0xd0
[ 292.534461] invoke_syscall+0x4c/0x118
[ 292.534467] el0_svc_common+0x98/0x104
[ 292.534473] do_el0_svc+0x30/0x80
[ 292.534478] el0_svc+0x20/0x50
[ 292.534481] el0t_64_sync_handler+0x78/0x108
[ 292.534485] el0t_64_sync+0x1a4/0x1a8
[ 292.534632] Kernel Offset: 0x1a5f80 from 0xffc00800
[ 292.534635] PHYS_OFFSET: 0x8000
[ 292.534638] CPU features: 0x40018541,a3300e42
[ 292.534644] Memory Limit: none
Cc: Rob Clark
Cc: Steven Rostedt
Cc: Ricardo Ribalda
Cc: Ross Zwisler
Signed-off-by: Joel Fernandes (Google)
---
drivers/gpu/drm/msm/adreno/adreno_device.c | 1 +
drivers/gpu/drm/msm/adreno/adreno_gpu.c| 2 +-
drivers/gpu/drm/msm/msm_gpu.h | 3 +++
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index f0cff62812c3..03d912dc0130 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -612,6 +612,7 @@ static void adreno_shutdown(struct platform_device *pdev)
{
struct msm_gpu *gpu = dev_to_gpu(>dev);
+ gpu->is_shutdown = true;
WARN_ON_ONCE(adreno_system_suspend(>dev));
}
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 382fb7f9e497..6903c6892469 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -251,7 +251,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct
msm_file_private *ctx,
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
/* No pointer params yet */
- if (*len != 0)
+ if (*len != 0 || gpu->is_shutdown)
return -EINVAL;
This will race with shutdown. Probably, propagating back the return
value of pm_runtime_get() in every possible ioctl call path is the right
thing to do.
I have never thought about this scenario. Do you know why userspace is
not freezed before kexec?
-Akhil.
switch (param) {
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index ff911e7305ce..f18b0a91442b 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -214,6 +214,9 @@ struct msm_gpu {
/* does gpu need hw_init? */
bool needs_hw_init;
+ /* is the GPU shutdown? */
+ bool is_shutdown;
+
/**
* global_faults: number of GPU hangs not attributed to a particular
* address space
[Freedreno] [PATCH 1/2] adreno: Shutdown the GPU properly
During kexec on ARM device, we notice that device_shutdown() only calls
pm_runtime_force_suspend() while shutting down the GPU. This means the GPU
kthread is still running and further, there maybe active submits.
This causes all kinds of issues during a kexec reboot:
Warning from shutdown path:
[ 292.509662] WARNING: CPU: 0 PID: 6304 at [...]
adreno_runtime_suspend+0x3c/0x44
[ 292.509863] Hardware name: Google Lazor (rev3 - 8) with LTE (DT)
[ 292.509872] pstate: 8049 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 292.509881] pc : adreno_runtime_suspend+0x3c/0x44
[ 292.509891] lr : pm_generic_runtime_suspend+0x30/0x44
[ 292.509905] sp : ffc014473bf0
[...]
[ 292.510043] Call trace:
[ 292.510051] adreno_runtime_suspend+0x3c/0x44
[ 292.510061] pm_generic_runtime_suspend+0x30/0x44
[ 292.510071] pm_runtime_force_suspend+0x54/0xc8
[ 292.510081] adreno_shutdown+0x1c/0x28
[ 292.510090] platform_shutdown+0x2c/0x38
[ 292.510104] device_shutdown+0x158/0x210
[ 292.510119] kernel_restart_prepare+0x40/0x4c
And here from GPU kthread, an SError OOPs:
[ 192.648789] el1h_64_error+0x7c/0x80
[ 192.648812] el1_interrupt+0x20/0x58
[ 192.648833] el1h_64_irq_handler+0x18/0x24
[ 192.648854] el1h_64_irq+0x7c/0x80
[ 192.648873] local_daif_inherit+0x10/0x18
[ 192.648900] el1h_64_sync_handler+0x48/0xb4
[ 192.648921] el1h_64_sync+0x7c/0x80
[ 192.648941] a6xx_gmu_set_oob+0xbc/0x1fc
[ 192.648968] a6xx_hw_init+0x44/0xe38
[ 192.648991] msm_gpu_hw_init+0x48/0x80
[ 192.649013] msm_gpu_submit+0x5c/0x1a8
[ 192.649034] msm_job_run+0xb0/0x11c
[ 192.649058] drm_sched_main+0x170/0x434
[ 192.649086] kthread+0x134/0x300
[ 192.649114] ret_from_fork+0x10/0x20
Fix by calling adreno_system_suspend() in the device_shutdown() path.
Cc: Rob Clark
Cc: Steven Rostedt
Cc: Ricardo Ribalda
Cc: Ross Zwisler
Signed-off-by: Joel Fernandes (Google)
---
drivers/gpu/drm/msm/adreno/adreno_device.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 24b489b6129a..f0cff62812c3 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -607,9 +607,12 @@ static int adreno_remove(struct platform_device *pdev)
return 0;
}
+static int adreno_system_suspend(struct device *dev);
static void adreno_shutdown(struct platform_device *pdev)
{
- pm_runtime_force_suspend(>dev);
+ struct msm_gpu *gpu = dev_to_gpu(>dev);
+
+ WARN_ON_ONCE(adreno_system_suspend(>dev));
}
static const struct of_device_id dt_match[] = {
--
2.38.1.493.g58b659f92b-goog
Re: [Freedreno] [PATCH 1/2] adreno: Shutdown the GPU properly
> On Nov 11, 2022, at 2:50 PM, Joel Fernandes (Google)
> wrote:
>
> During kexec on ARM device, we notice that device_shutdown() only calls
> pm_runtime_force_suspend() while shutting down the GPU. This means the GPU
> kthread is still running and further, there maybe active submits.
>
> This causes all kinds of issues during a kexec reboot:
>
> Warning from shutdown path:
>
> [ 292.509662] WARNING: CPU: 0 PID: 6304 at [...]
> adreno_runtime_suspend+0x3c/0x44
> [ 292.509863] Hardware name: Google Lazor (rev3 - 8) with LTE (DT)
> [ 292.509872] pstate: 8049 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 292.509881] pc : adreno_runtime_suspend+0x3c/0x44
> [ 292.509891] lr : pm_generic_runtime_suspend+0x30/0x44
> [ 292.509905] sp : ffc014473bf0
> [...]
> [ 292.510043] Call trace:
> [ 292.510051] adreno_runtime_suspend+0x3c/0x44
> [ 292.510061] pm_generic_runtime_suspend+0x30/0x44
> [ 292.510071] pm_runtime_force_suspend+0x54/0xc8
> [ 292.510081] adreno_shutdown+0x1c/0x28
> [ 292.510090] platform_shutdown+0x2c/0x38
> [ 292.510104] device_shutdown+0x158/0x210
> [ 292.510119] kernel_restart_prepare+0x40/0x4c
>
> And here from GPU kthread, an SError OOPs:
>
> [ 192.648789] el1h_64_error+0x7c/0x80
> [ 192.648812] el1_interrupt+0x20/0x58
> [ 192.648833] el1h_64_irq_handler+0x18/0x24
> [ 192.648854] el1h_64_irq+0x7c/0x80
> [ 192.648873] local_daif_inherit+0x10/0x18
> [ 192.648900] el1h_64_sync_handler+0x48/0xb4
> [ 192.648921] el1h_64_sync+0x7c/0x80
> [ 192.648941] a6xx_gmu_set_oob+0xbc/0x1fc
> [ 192.648968] a6xx_hw_init+0x44/0xe38
> [ 192.648991] msm_gpu_hw_init+0x48/0x80
> [ 192.649013] msm_gpu_submit+0x5c/0x1a8
> [ 192.649034] msm_job_run+0xb0/0x11c
> [ 192.649058] drm_sched_main+0x170/0x434
> [ 192.649086] kthread+0x134/0x300
> [ 192.649114] ret_from_fork+0x10/0x20
>
> Fix by calling adreno_system_suspend() in the device_shutdown() path.
>
> Cc: Rob Clark
> Cc: Steven Rostedt
> Cc: Ricardo Ribalda
> Cc: Ross Zwisler
> Signed-off-by: Joel Fernandes (Google)
> ---
> drivers/gpu/drm/msm/adreno/adreno_device.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 24b489b6129a..f0cff62812c3 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -607,9 +607,12 @@ static int adreno_remove(struct platform_device *pdev)
>return 0;
> }
>
> +static int adreno_system_suspend(struct device *dev);
> static void adreno_shutdown(struct platform_device *pdev)
> {
> -pm_runtime_force_suspend(>dev);
> +struct msm_gpu *gpu = dev_to_gpu(>dev);
> +
This local variable definition should go to patch 2/2. Will fix in v2.
Thanks,
- Joel
> +WARN_ON_ONCE(adreno_system_suspend(>dev));
> }
>
> static const struct of_device_id dt_match[] = {
> --
> 2.38.1.493.g58b659f92b-goog
>
[Freedreno] [PATCH 2/2] adreno: Detect shutdown during get_param()
Even though the GPU is shut down, during kexec reboot we can have userspace
still running. This is especially true if KEXEC_JUMP is not enabled, because we
do not freeze userspace in this case.
To prevent crashes, track that the GPU is shutdown and prevent get_param() from
accessing GPU resources if we find it shutdown.
This fixes the following crash during kexec reboot on an ARM64 device with
adreno GPU:
[ 292.534314] Kernel panic - not syncing: Asynchronous SError Interrupt
[ 292.534323] Hardware name: Google Lazor (rev3 - 8) with LTE (DT)
[ 292.534326] Call trace:
[ 292.534328] dump_backtrace+0x0/0x1d4
[ 292.534337] show_stack+0x20/0x2c
[ 292.534342] dump_stack_lvl+0x60/0x78
[ 292.534347] dump_stack+0x18/0x38
[ 292.534352] panic+0x148/0x3b0
[ 292.534357] nmi_panic+0x80/0x94
[ 292.534364] arm64_serror_panic+0x70/0x7c
[ 292.534369] do_serror+0x0/0x7c
[ 292.534372] do_serror+0x54/0x7c
[ 292.534377] el1h_64_error_handler+0x34/0x4c
[ 292.534381] el1h_64_error+0x7c/0x80
[ 292.534386] el1_interrupt+0x20/0x58
[ 292.534389] el1h_64_irq_handler+0x18/0x24
[ 292.534395] el1h_64_irq+0x7c/0x80
[ 292.534399] local_daif_inherit+0x10/0x18
[ 292.534405] el1h_64_sync_handler+0x48/0xb4
[ 292.534410] el1h_64_sync+0x7c/0x80
[ 292.534414] a6xx_gmu_set_oob+0xbc/0x1fc
[ 292.534422] a6xx_get_timestamp+0x40/0xb4
[ 292.534426] adreno_get_param+0x12c/0x1e0
[ 292.534433] msm_ioctl_get_param+0x64/0x70
[ 292.534440] drm_ioctl_kernel+0xe8/0x158
[ 292.534448] drm_ioctl+0x208/0x320
[ 292.534453] __arm64_sys_ioctl+0x98/0xd0
[ 292.534461] invoke_syscall+0x4c/0x118
[ 292.534467] el0_svc_common+0x98/0x104
[ 292.534473] do_el0_svc+0x30/0x80
[ 292.534478] el0_svc+0x20/0x50
[ 292.534481] el0t_64_sync_handler+0x78/0x108
[ 292.534485] el0t_64_sync+0x1a4/0x1a8
[ 292.534632] Kernel Offset: 0x1a5f80 from 0xffc00800
[ 292.534635] PHYS_OFFSET: 0x8000
[ 292.534638] CPU features: 0x40018541,a3300e42
[ 292.534644] Memory Limit: none
Cc: Rob Clark
Cc: Steven Rostedt
Cc: Ricardo Ribalda
Cc: Ross Zwisler
Signed-off-by: Joel Fernandes (Google)
---
drivers/gpu/drm/msm/adreno/adreno_device.c | 1 +
drivers/gpu/drm/msm/adreno/adreno_gpu.c| 2 +-
drivers/gpu/drm/msm/msm_gpu.h | 3 +++
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index f0cff62812c3..03d912dc0130 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -612,6 +612,7 @@ static void adreno_shutdown(struct platform_device *pdev)
{
struct msm_gpu *gpu = dev_to_gpu(>dev);
+ gpu->is_shutdown = true;
WARN_ON_ONCE(adreno_system_suspend(>dev));
}
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 382fb7f9e497..6903c6892469 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -251,7 +251,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct
msm_file_private *ctx,
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
/* No pointer params yet */
- if (*len != 0)
+ if (*len != 0 || gpu->is_shutdown)
return -EINVAL;
switch (param) {
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index ff911e7305ce..f18b0a91442b 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -214,6 +214,9 @@ struct msm_gpu {
/* does gpu need hw_init? */
bool needs_hw_init;
+ /* is the GPU shutdown? */
+ bool is_shutdown;
+
/**
* global_faults: number of GPU hangs not attributed to a particular
* address space
--
2.38.1.493.g58b659f92b-goog
Re: [Freedreno] [PATCH v2.5] drm/msm/dsi: switch to DRM_PANEL_BRIDGE
Hi,
This patch has caused a regression on 6.1-rc for some devices that use
DSI panels. The new behaviour results in the DSI controller being
switched off before the panel unprepare hook is called. As a result,
panel drivers which call mipi_dsi_dcs_write() or similar in
unprepare() fail.
I've noticed it specifically on the OnePlus 6 (with upstream Samsung
s0fef00 panel driver) and the SHIFT6mq with an out of tree driver.
On 12/07/2022 14:22, Dmitry Baryshkov wrote:
Currently the DSI driver has two separate paths: one if the next device
in a chain is a bridge and another one if the panel is connected
directly to the DSI host. Simplify the code path by using panel-bridge
driver (already selected in Kconfig) and dropping support for
handling the panel directly.
Signed-off-by: Dmitry Baryshkov
---
I'm not sending this as a separate patchset (I'd like to sort out mdp5
first), but more of a preview of changes related to
msm_dsi_manager_ext_bridge_init().
---
drivers/gpu/drm/msm/dsi/dsi.c | 35 +---
drivers/gpu/drm/msm/dsi/dsi.h | 16 +-
drivers/gpu/drm/msm/dsi/dsi_host.c| 25 ---
drivers/gpu/drm/msm/dsi/dsi_manager.c | 283 +++---
4 files changed, 36 insertions(+), 323 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 1625328fa430..4edb9167e600 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -6,14 +6,6 @@
#include "dsi.h"
#include "dsi_cfg.h"
-struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi)
-{
- if (!msm_dsi || !msm_dsi_device_connected(msm_dsi))
- return NULL;
-
- return msm_dsi->encoder;
-}
-
bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi)
{
unsigned long host_flags = msm_dsi_host_get_mode_flags(msm_dsi->host);
@@ -220,7 +212,6 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
drm_device *dev,
struct drm_encoder *encoder)
{
struct msm_drm_private *priv;
- struct drm_bridge *ext_bridge;
int ret;
if (WARN_ON(!encoder) || WARN_ON(!msm_dsi) || WARN_ON(!dev))
@@ -254,26 +245,10 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
drm_device *dev,
goto fail;
}
- /*
-* check if the dsi encoder output is connected to a panel or an
-* external bridge. We create a connector only if we're connected to a
-* drm_panel device. When we're connected to an external bridge, we
-* assume that the drm_bridge driver will create the connector itself.
-*/
- ext_bridge = msm_dsi_host_get_bridge(msm_dsi->host);
-
- if (ext_bridge)
- msm_dsi->connector =
- msm_dsi_manager_ext_bridge_init(msm_dsi->id);
- else
- msm_dsi->connector =
- msm_dsi_manager_connector_init(msm_dsi->id);
-
- if (IS_ERR(msm_dsi->connector)) {
- ret = PTR_ERR(msm_dsi->connector);
+ ret = msm_dsi_manager_ext_bridge_init(msm_dsi->id);
+ if (ret) {
DRM_DEV_ERROR(dev->dev,
"failed to create dsi connector: %d\n", ret);
- msm_dsi->connector = NULL;
goto fail;
}
@@ -287,12 +262,6 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
drm_device *dev,
msm_dsi->bridge = NULL;
}
- /* don't destroy connector if we didn't make it */
- if (msm_dsi->connector && !msm_dsi->external_bridge)
- msm_dsi->connector->funcs->destroy(msm_dsi->connector);
-
- msm_dsi->connector = NULL;
-
return ret;
}
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 580a1e6358bf..703e4c88d7fb 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -12,7 +12,6 @@
#include
#include
#include
-#include
#include "msm_drv.h"
#include "disp/msm_disp_snapshot.h"
@@ -49,8 +48,6 @@ struct msm_dsi {
struct drm_device *dev;
struct platform_device *pdev;
- /* connector managed by us when we're connected to a drm_panel */
- struct drm_connector *connector;
/* internal dsi bridge attached to MDP interface */
struct drm_bridge *bridge;
@@ -58,10 +55,8 @@ struct msm_dsi {
struct msm_dsi_phy *phy;
/*
-* panel/external_bridge connected to dsi bridge output, only one of the
-* two can be valid at a time
+* external_bridge connected to dsi bridge output
*/
- struct drm_panel *panel;
struct drm_bridge *external_bridge;
struct device *phy_dev;
@@ -76,8 +71,7 @@ struct msm_dsi {
/* dsi manager */
struct drm_bridge *msm_dsi_manager_bridge_init(u8 id);
void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge);
-struct drm_connector *msm_dsi_manager_connector_init(u8 id);
-struct drm_connector
Re: [Freedreno] [PATCH v1 9/9] arm64: dts: qcom: sm8350-hdk: Enable lt9611uxc dsi-hdmi bridge
On Sat, 29 Oct 2022 at 00:06, Krzysztof Kozlowski
wrote:
>
> On 28/10/2022 08:08, Robert Foss wrote:
> > The sm8350-hdk ships with the LT9611 UXC DSI/HDMI bridge chip.
> >
> > In order to toggle the board to enable the HDMI output,
> > switch #7 & #8 on the rightmost multi-switch package have
> > to be toggled to On.
> >
> > Signed-off-by: Robert Foss
> > ---
> > arch/arm64/boot/dts/qcom/sm8350-hdk.dts | 106
> > 1 file changed, 106 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > index 6e07feb4b3b2..b38b58f8 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > +++ b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > @@ -20,6 +20,17 @@ chosen {
> > stdout-path = "serial0:115200n8";
> > };
> >
> > + hdmi-out {
>
> Generic node names, so hdmi-connector or just connector.
Ack.
>
> > + compatible = "hdmi-connector";
> > + type = "a";
> > +
> > + port {
> > + hdmi_con: endpoint {
> > + remote-endpoint = <_out>;
> > + };
> > + };
> > + };
> > +
> > vph_pwr: vph-pwr-regulator {
> > compatible = "regulator-fixed";
> > regulator-name = "vph_pwr";
> > @@ -29,6 +40,32 @@ vph_pwr: vph-pwr-regulator {
> > regulator-always-on;
> > regulator-boot-on;
> > };
> > +
> > + lt9611_1v2: lt9611-1v2 {
>
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Ack.
>
> > + compatible = "regulator-fixed";
> > + regulator-name = "LT9611_1V2";
> > +
> > + vin-supply = <_pwr>;
> > + regulator-min-microvolt = <120>;
> > + regulator-max-microvolt = <120>;
> > + gpio = < 49 GPIO_ACTIVE_HIGH>;
> > + enable-active-high;
> > + regulator-boot-on;
> > + regulator-always-on;
> > + };
> > +
> > + lt9611_3v3: lt9611-3v3 {
>
> Ditto
Ack.
>
>
> > + compatible = "regulator-fixed";
> > + regulator-name = "LT9611_3V3";
> > +
> > + vin-supply = <_bob>;
> > + gpio = < 47 GPIO_ACTIVE_HIGH>;
> > + regulator-min-microvolt = <330>;
> > + regulator-max-microvolt = <330>;
> > + enable-active-high;
> > + regulator-boot-on;
> > + regulator-always-on;
> > + };
> > };
> >
> > {
> > @@ -220,6 +257,15 @@ {
> > {
> > status = "okay";
> > vdda-supply = <_l6b_1p2>;
> > +
> > + ports {
> > + port@1 {
> > + endpoint {
> > + remote-endpoint = <_a>;
> > + data-lanes = <0 1 2 3>;
> > + };
> > + };
> > + };
> > };
> >
> > _phy {
> > @@ -231,6 +277,48 @@ _dma1 {
> > status = "okay";
> > };
> >
> > + {
> > + status = "okay";
>
> status is the last property
Ack.
>
> > + clock-frequency = <40>;
> > +
> > + lt9611_codec: hdmi-bridge@2b {
> > + compatible = "lontium,lt9611uxc";
> > + reg = <0x2b>;
> > + status = "okay";
>
> Why status?
It should be removed. Fixing in v2.
>
> > +
> > + interrupts-extended = < 50 IRQ_TYPE_EDGE_FALLING>;
> > + reset-gpios = < 48 GPIO_ACTIVE_HIGH>;
> > +
> > + vdd-supply = <_1v2>;
> > + vcc-supply = <_3v3>;
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <_irq_pin _rst_pin>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + lt9611_a: endpoint {
> > + remote-endpoint = <_out>;
> > + };
> > + };
> > +
> > + port@2 {
> > + reg = <2>;
> > +
> > + lt9611_out: endpoint {
> > + remote-endpoint = <_con>;
> > + };
> > + };
> > +
>
> No need for blank line
Ack
>
> > + };
> > + };
> > +};
> > +
> > {
> > status = "okay";
> > };
> > @@ -248,6 +336,10 @@ _id_0 {
> > status = "okay";
> > };
> >
> > +_id_2 {
> > + status = "okay";
> > +};
> > +
> > {
> > status = "okay";
> > firmware-name = "qcom/sm8350/slpi.mbn";
> > @@ -544,4 +636,18 @@ usb_hub_enabled_state: usb-hub-enabled-state {
> > drive-strength = <2>;
> > output-low;
> > };
> > +
> > + lt9611_rst_pin: lt9611-rst-state {
> > +
Re: [Freedreno] [PATCH v1 9/9] arm64: dts: qcom: sm8350-hdk: Enable lt9611uxc dsi-hdmi bridge
On Fri, 28 Oct 2022 at 15:57, Bjorn Andersson wrote:
>
> On Fri, Oct 28, 2022 at 02:08:12PM +0200, Robert Foss wrote:
> > The sm8350-hdk ships with the LT9611 UXC DSI/HDMI bridge chip.
> >
> > In order to toggle the board to enable the HDMI output,
> > switch #7 & #8 on the rightmost multi-switch package have
> > to be toggled to On.
> >
> > Signed-off-by: Robert Foss
> > ---
> > arch/arm64/boot/dts/qcom/sm8350-hdk.dts | 106
> > 1 file changed, 106 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > index 6e07feb4b3b2..b38b58f8 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > +++ b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > @@ -20,6 +20,17 @@ chosen {
> > stdout-path = "serial0:115200n8";
> > };
> >
> > + hdmi-out {
> > + compatible = "hdmi-connector";
> > + type = "a";
> > +
> > + port {
> > + hdmi_con: endpoint {
> > + remote-endpoint = <_out>;
> > + };
> > + };
> > + };
> > +
> > vph_pwr: vph-pwr-regulator {
> > compatible = "regulator-fixed";
> > regulator-name = "vph_pwr";
> > @@ -29,6 +40,32 @@ vph_pwr: vph-pwr-regulator {
> > regulator-always-on;
> > regulator-boot-on;
> > };
> > +
> > + lt9611_1v2: lt9611-1v2 {
> > + compatible = "regulator-fixed";
> > + regulator-name = "LT9611_1V2";
> > +
> > + vin-supply = <_pwr>;
> > + regulator-min-microvolt = <120>;
> > + regulator-max-microvolt = <120>;
> > + gpio = < 49 GPIO_ACTIVE_HIGH>;
> > + enable-active-high;
> > + regulator-boot-on;
> > + regulator-always-on;
>
> Why is this always-on?
It shouldn't be. Removing this in v2.
>
> > + };
> > +
> > + lt9611_3v3: lt9611-3v3 {
> > + compatible = "regulator-fixed";
> > + regulator-name = "LT9611_3V3";
> > +
> > + vin-supply = <_bob>;
> > + gpio = < 47 GPIO_ACTIVE_HIGH>;
> > + regulator-min-microvolt = <330>;
> > + regulator-max-microvolt = <330>;
> > + enable-active-high;
> > + regulator-boot-on;
> > + regulator-always-on;
> > + };
> > };
> >
> > {
> > @@ -220,6 +257,15 @@ {
> > {
> > status = "okay";
> > vdda-supply = <_l6b_1p2>;
> > +
> > + ports {
> > + port@1 {
> > + endpoint {
> > + remote-endpoint = <_a>;
> > + data-lanes = <0 1 2 3>;
> > + };
> > + };
> > + };
> > };
> >
> > _phy {
> > @@ -231,6 +277,48 @@ _dma1 {
> > status = "okay";
> > };
> >
> > + {
> > + status = "okay";
>
> Please keep status last. (Yes I see that it goes against the convention
> in this file, so let's update that at some point as well)
Ack.
>
> > + clock-frequency = <40>;
> > +
> > + lt9611_codec: hdmi-bridge@2b {
> > + compatible = "lontium,lt9611uxc";
> > + reg = <0x2b>;
> > + status = "okay";
>
> This is the default, you can omit it.
Ack.
>
> > +
> > + interrupts-extended = < 50 IRQ_TYPE_EDGE_FALLING>;
> > + reset-gpios = < 48 GPIO_ACTIVE_HIGH>;
> > +
> > + vdd-supply = <_1v2>;
> > + vcc-supply = <_3v3>;
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <_irq_pin _rst_pin>;
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + lt9611_a: endpoint {
> > + remote-endpoint = <_out>;
> > + };
> > + };
> > +
> > + port@2 {
> > + reg = <2>;
> > +
> > + lt9611_out: endpoint {
> > + remote-endpoint = <_con>;
> > + };
> > + };
> > +
> > + };
> > + };
> > +};
> > +
> > {
> > status = "okay";
> > };
> > @@ -248,6 +336,10 @@ _id_0 {
> > status = "okay";
> > };
> >
> > +_id_2 {
> > + status = "okay";
> > +};
> > +
> > {
> > status = "okay";
> > firmware-name = "qcom/sm8350/slpi.mbn";
> > @@ -544,4 +636,18 @@ usb_hub_enabled_state: usb-hub-enabled-state {
> > drive-strength = <2>;
> > output-low;
> > };
> > +
> > + lt9611_rst_pin: lt9611-rst-state {
> > + pins = "gpio48";
> > + function = "normal";
> > +
> > + output-high;
> > +
Re: [Freedreno] [PATCH v1 8/9] arm64: dts: qcom: sm8350-hdk: Enable display & dsi nodes
On Sat, 29 Oct 2022 at 00:03, Krzysztof Kozlowski
wrote:
>
> On 28/10/2022 08:08, Robert Foss wrote:
> > Enable the display subsystem and the dsi0 output for
> > the sm8350-hdk board.
> >
> > Signed-off-by: Robert Foss
> > ---
> > arch/arm64/boot/dts/qcom/sm8350-hdk.dts | 22 ++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > index e6deb08c6da0..6e07feb4b3b2 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > +++ b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
> > @@ -213,10 +213,32 @@ {
> > firmware-name = "qcom/sm8350/cdsp.mbn";
> > };
> >
> > + {
> > + status = "okay";
> > +};
> > +
> > + {
> > + status = "okay";
>
> Status is the last property.
Ack.
>
>
> Best regards,
> Krzysztof
>
Re: [Freedreno] [PATCH v1 7/9] arm64: dts: qcom: sm8350: Add display system nodes
On Sat, 29 Oct 2022 at 00:01, Krzysztof Kozlowski
wrote:
>
> On 28/10/2022 08:08, Robert Foss wrote:
> > Add mdss, mdss_mdp, dsi0, dsi0_phy nodes. With these
> > nodes the display subsystem is configured to support
> > one DSI output.
> >
> > Signed-off-by: Robert Foss
> > ---
> > arch/arm64/boot/dts/qcom/sm8350.dtsi | 196 ++-
> > 1 file changed, 192 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > index b6e44cd3b394..eaa3cdee1860 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > @@ -3,6 +3,7 @@
> > * Copyright (c) 2020, Linaro Limited
> > */
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -2535,14 +2536,200 @@ usb_2_dwc3: usb@a80 {
> > };
> > };
> >
> > + mdss: mdss@ae0 {
> > + compatible = "qcom,sm8350-mdss";
> > + reg = <0 0x0ae0 0 0x1000>;
> > + reg-names = "mdss";
> > +
> > + interconnects = <_noc MASTER_MDP0 0 _virt
> > SLAVE_EBI1 0>,
> > + <_noc MASTER_MDP1 0 _virt
> > SLAVE_EBI1 0>;
> > + interconnect-names = "mdp0-mem", "mdp1-mem";
> > +
> > + power-domains = < MDSS_GDSC>;
> > + resets = < DISP_CC_MDSS_CORE_BCR>;
> > +
> > + clocks = < DISP_CC_MDSS_AHB_CLK>,
> > + < GCC_DISP_HF_AXI_CLK>,
> > + < GCC_DISP_SF_AXI_CLK>,
> > + < DISP_CC_MDSS_MDP_CLK>;
> > + clock-names = "iface", "bus", "nrt_bus", "core";
> > +
> > + interrupts = ;
> > + interrupt-controller;
> > + #interrupt-cells = <1>;
> > +
> > + status = "ok";
>
> No need for this.
Ack, I'll switch to disabled.
>
> > +
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
> > +
> > + mdss_mdp: mdp@ae01000 {
>
> Node name: display-controller
Ack.
> > + compatible = "qcom,sm8350-dpu";
> > + reg = <0 0x0ae01000 0 0x8f000>,
> > + <0 0x0aeb 0 0x2008>;
> > + reg-names = "mdp", "vbif";
> > + iommus = <_smmu 0x820 0x402>;
> > +
> > + clocks = < GCC_DISP_HF_AXI_CLK>,
> > + < GCC_DISP_SF_AXI_CLK>,
> > + < DISP_CC_MDSS_AHB_CLK>,
> > + < DISP_CC_MDSS_MDP_LUT_CLK>,
> > + < DISP_CC_MDSS_MDP_CLK>,
> > + < DISP_CC_MDSS_VSYNC_CLK>;
> > + clock-names = "bus",
> > + "nrt_bus",
> > + "iface",
> > + "lut",
> > + "core",
> > + "vsync";
> > +
> > + assigned-clocks = <
> > DISP_CC_MDSS_VSYNC_CLK>;
> > + assigned-clock-rates = <1920>;
> > +
> > + operating-points-v2 = <_opp_table>;
> > + power-domains = < SM8350_MMCX>;
> > +
> > + interrupt-parent = <>;
> > + interrupts = <0>;
> > +
> > + status = "ok";
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > + dpu_intf1_out: endpoint {
> > + remote-endpoint =
> > <_in>;
> > + };
> > + };
> > + };
> > +
> > + mdp_opp_table: mdp-opp-table {
>
> I have doubts that it passes dtbs_checks... opp-table
Ack.
>
> > + compatible = "operating-points-v2";
> > +
> > + opp-2 {
> > + opp-hz = /bits/ 64
> > <2>;
> > + required-opps =
> > <_opp_low_svs>;
> > + };
> > +
> > + opp-3 {
> > +
[Freedreno] [v1] drm/msm/disp/dpu1: add color management support for the crtc
Add color management support for the crtc provided there are
enough dspps that can be allocated from the catalogue.
Changes in v1:
- cache color enabled state in the dpu crtc obj (Dmitry)
- simplify dspp allocation while creating crtc (Dmitry)
- register for color when crtc is created (Dmitry)
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 5 +--
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h| 6 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 53 -
4 files changed, 64 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 4170fbe..ca4c3b3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1571,7 +1571,7 @@ static const struct drm_crtc_helper_funcs
dpu_crtc_helper_funcs = {
/* initialize crtc */
struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
- struct drm_plane *cursor)
+ struct drm_plane *cursor, unsigned long
features)
{
struct drm_crtc *crtc = NULL;
struct dpu_crtc *dpu_crtc = NULL;
@@ -1583,6 +1583,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev,
struct drm_plane *plane,
crtc = _crtc->base;
crtc->dev = dev;
+ dpu_crtc->color_enabled = features & BIT(DPU_DSPP_PCC);
spin_lock_init(_crtc->spin_lock);
atomic_set(_crtc->frame_pending, 0);
@@ -1604,7 +1605,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev,
struct drm_plane *plane,
drm_crtc_helper_add(crtc, _crtc_helper_funcs);
- drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
+ drm_crtc_enable_color_mgmt(crtc, 0, dpu_crtc->color_enabled, 0);
/* save user friendly CRTC name for later */
snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u", crtc->base.id);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 539b68b..342f9ae 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -136,6 +136,7 @@ struct dpu_crtc_frame_event {
* @enabled : whether the DPU CRTC is currently enabled. updated in the
* commit-thread, not state-swap time which is earlier, so
* safe to make decisions on during VBLANK on/off work
+ * @color_enabled : whether crtc supports color management
* @feature_list : list of color processing features supported on a crtc
* @active_list : list of color processing features are active
* @dirty_list: list of color processing features are dirty
@@ -164,7 +165,7 @@ struct dpu_crtc {
u64 play_count;
ktime_t vblank_cb_time;
bool enabled;
-
+ bool color_enabled;
struct list_head feature_list;
struct list_head active_list;
struct list_head dirty_list;
@@ -269,10 +270,11 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc);
* @dev: dpu device
* @plane: base plane
* @cursor: cursor plane
+ * @features: color features
* @Return: new crtc object or error
*/
struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
- struct drm_plane *cursor);
+ struct drm_plane *cursor, unsigned long
features);
/**
* dpu_crtc_register_custom_event - api for enabling/disabling crtc event
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index c9058aa..d72edb8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -579,6 +579,7 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
static struct msm_display_topology dpu_encoder_get_topology(
struct dpu_encoder_virt *dpu_enc,
struct dpu_kms *dpu_kms,
+ struct dpu_crtc *dpu_crtc,
struct drm_display_mode *mode)
{
struct msm_display_topology topology = {0};
@@ -607,7 +608,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
else
topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
- if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
+ if (dpu_crtc->color_enabled) {
if (dpu_kms->catalog->dspp &&
(dpu_kms->catalog->dspp_count >= topology.num_lm))
topology.num_dspp = topology.num_lm;
@@ -642,6 +643,7 @@ static int dpu_encoder_virt_atomic_check(
struct drm_display_mode *adj_mode;
struct msm_display_topology topology;
struct dpu_global_state *global_state;
+ struct dpu_crtc *dpu_crtc;
int i = 0;
int ret = 0;
@@ -652,6 +654,7 @@ static int
[Freedreno] [v1] drm/msm/disp/dpu1: populate disp_info with connector type
Populate disp_info with connector type. Since DRM encoder type
for few encoders can be similar (like eDP and DP) this information
will be useful to differentiate interfaces.
Changes in v1:
- add connector type in the disp_info (Dmitry)
- add helper functions to know encoder type
- update commit text reflecting the change
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 26 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 ++
drivers/gpu/drm/msm/dp/dp_display.c | 5
drivers/gpu/drm/msm/msm_drv.h | 7 -
5 files changed, 77 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b..c9058aa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -217,6 +217,40 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
};
+bool dpu_encoder_is_external(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+
+ if (!drm_enc)
+ return false;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ return (dpu_enc->disp_info.connector_type ==
+ DRM_MODE_CONNECTOR_DisplayPort);
+}
+
+bool dpu_encoder_is_virtual(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+
+ if (!drm_enc)
+ return false;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ return (dpu_enc->disp_info.connector_type ==
DRM_MODE_CONNECTOR_WRITEBACK);
+}
+
+bool dpu_encoder_is_primary(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc;
+
+ if (!drm_enc)
+ return false;
+
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+ return((dpu_enc->disp_info.connector_type == DRM_MODE_CONNECTOR_DSI) ||
+ (dpu_enc->disp_info.connector_type == DRM_MODE_CONNECTOR_eDP));
+}
bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
{
@@ -2412,7 +2446,7 @@ int dpu_encoder_setup(struct drm_device *dev, struct
drm_encoder *enc,
struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
struct drm_encoder *drm_enc = NULL;
struct dpu_encoder_virt *dpu_enc = NULL;
- int ret = 0;
+ int ret = 0, intf_i;
dpu_enc = to_dpu_encoder_virt(enc);
@@ -2424,13 +2458,17 @@ int dpu_encoder_setup(struct drm_device *dev, struct
drm_encoder *enc,
timer_setup(_enc->frame_done_timer,
dpu_encoder_frame_done_timeout, 0);
+ intf_i = disp_info->h_tile_instance[0];
if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
timer_setup(_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
0);
- else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+ else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
- priv->dp[disp_info->h_tile_instance[0]]);
+ priv->dp[intf_i]);
+ disp_info->connector_type =
+ msm_dp_get_connector_type(priv->dp[intf_i]);
+ }
INIT_DELAYED_WORK(_enc->delayed_off_work,
dpu_encoder_off_work);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236e..d361c5d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -25,16 +25,18 @@
* @num_of_h_tiles: Number of horizontal tiles in case of split interface
* @h_tile_instance:Controller instance used per tile. Number of elements
is
* based on num_of_h_tiles
- * @is_cmd_modeBoolean to indicate if the CMD mode is requested
+ * @is_cmd_mode:Boolean to indicate if the CMD mode is requested
+ * @connector_type: DRM_MODE_CONNECTOR_ type
* @is_te_using_watchdog_timer: Boolean to indicate watchdog TE is
- * used instead of panel TE in cmd mode panels
- * @dsc: DSC configuration data for DSC-enabled displays
+ * used instead of panel TE in cmd mode panels
+ * @dsc:DSC configuration data for DSC-enabled displays
*/
struct msm_display_info {
int intf_type;
uint32_t num_of_h_tiles;
uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
bool is_cmd_mode;
+ int connector_type;
bool is_te_using_watchdog_timer;
struct drm_dsc_config *dsc;
};
@@ -224,4 +226,22 @@ void dpu_encoder_cleanup_wb_job(struct drm_encoder
*drm_enc,
*/
bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc);
+/**
+*
[Freedreno] [v1] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder
Pin each crtc with one encoder. This arrangement will
disallow crtc switching between encoders and also will
facilitate to advertise certain features on crtc based
on encoder type.
Changes in v1:
- use drm_for_each_encoder macro while iterating through
encoder list (Dmitry)
Signed-off-by: Kalyan Thota
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7a5fabc..0d94eec0d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -798,19 +798,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
max_crtc_count = min(max_crtc_count, primary_planes_idx);
/* Create one CRTC per encoder */
- for (i = 0; i < max_crtc_count; i++) {
- crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
- if (IS_ERR(crtc)) {
- ret = PTR_ERR(crtc);
- return ret;
+ i = 0;
+ drm_for_each_encoder(encoder, dev) {
+ if (i < max_crtc_count) {
+ crtc = dpu_crtc_init(dev, primary_planes[i],
cursor_planes[i]);
+ if (IS_ERR(crtc)) {
+ ret = PTR_ERR(crtc);
+ return ret;
+ }
+ priv->crtcs[priv->num_crtcs++] = crtc;
+ encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
}
- priv->crtcs[priv->num_crtcs++] = crtc;
+ i++;
}
- /* All CRTCs are compatible with all encoders */
- drm_for_each_encoder(encoder, dev)
- encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
-
return 0;
}
--
2.7.4
[Freedreno] [v9] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280
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)
Changes in v7:
- Cache flush mask per dspp (Dmitry)
- Few nits (Marijn)
Changes in v8:
- Few nits (Marijn)
Changes in v9:
- use DSPP enum while accessing flush mask to make it readable (Dmitry)
- Few nits (Dmitry)
Signed-off-by: Kalyan Thota
---
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 | 64 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 5 +-
5 files changed, 65 insertions(+), 15 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..126ee37 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..0ee8220 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -28,22 +28,23 @@
#define CTL_INTF_ACTIVE 0x0F4
#define CTL_MERGE_3D_FLUSH0x100
#define CTL_DSC_ACTIVE0x0E8
-#define CTL_DSC_FLUSH0x104
+#define CTL_DSC_FLUSH 0x104
#define CTL_WB_FLUSH 0x108
#define CTL_INTF_FLUSH0x110
#define CTL_INTF_MASTER 0x134
#define CTL_FETCH_PIPE_ACTIVE 0x0FC
+#define CTL_DSPP_n_FLUSH(n) ((0x13C) + ((n) * 4))
-#define CTL_MIXER_BORDER_OUTBIT(24)
-#define CTL_FLUSH_MASK_CTL BIT(17)
+#define CTL_MIXER_BORDER_OUT BIT(24)
+#define CTL_FLUSH_MASK_CTLBIT(17)
-#define DPU_REG_RESET_TIMEOUT_US2000
-#define MERGE_3D_IDX 23
-#define DSC_IDX22
-#define INTF_IDX 31
-#define WB_IDX 16
-#define CTL_INVALID_BIT 0x
-#define CTL_DEFAULT_GROUP_ID 0xf
+#define DPU_REG_RESET_TIMEOUT_US 2000
+#define MERGE_3D_IDX 23
+#define DSC_IDX 22
+#define INTF_IDX 31
+#define WB_IDX16
+#define
Re: [Freedreno] [PATCH 4/4] drm/msm/disp/dpu1: add color management support for the crtc
>-Original Message-
>From: Dmitry Baryshkov
>Sent: Wednesday, November 9, 2022 6:02 PM
>To: Kalyan Thota (QUIC) ; dri-
>de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
>freedreno@lists.freedesktop.org; devicet...@vger.kernel.org
>Cc: linux-ker...@vger.kernel.org; robdcl...@chromium.org;
>diand...@chromium.org; swb...@chromium.org; Vinod Polimera (QUIC)
>; Abhinav Kumar (QUIC)
>
>Subject: Re: [PATCH 4/4] drm/msm/disp/dpu1: add color management support
>for the crtc
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 09/11/2022 15:16, Kalyan Thota wrote:
>> Add color management support for the crtc provided there are enough
>> dspps that can be allocated from the catalogue
>>
>> Signed-off-by: Kalyan Thota
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 15 ++--
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h| 6
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 53
>+
>> 4 files changed, 77 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 4170fbe..6bd3a64 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -18,9 +18,11 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> +#include "../../../drm_crtc_internal.h"
>
>If it's internal, it is not supposed to be used by other parties, including
>the msm
>drm. At least a comment why you are including this file would be helpful.
>
>>
>> #include "dpu_kms.h"
>> #include "dpu_hw_lm.h"
>> @@ -553,6 +555,17 @@ static void _dpu_crtc_complete_flip(struct drm_crtc
>*crtc)
>> spin_unlock_irqrestore(>event_lock, flags);
>> }
>>
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc) {
>> + u32 ctm_id = crtc->dev->mode_config.ctm_property->base.id;
>> + u32 gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id;
>> + u32 degamma_id =
>> +crtc->dev->mode_config.degamma_lut_property->base.id;
>> +
>> + return !!(drm_mode_obj_find_prop_id(>base, ctm_id) ||
>> +drm_mode_obj_find_prop_id(>base, gamma_id) ||
>> +drm_mode_obj_find_prop_id(>base, degamma_id));
>> +}
>> +
>> enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>> {
>> struct drm_encoder *encoder;
>> @@ -1604,8 +1617,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
>> *dev, struct drm_plane *plane,
>>
>> drm_crtc_helper_add(crtc, _crtc_helper_funcs);
>>
>> - drm_crtc_enable_color_mgmt(crtc, 0, true, 0);
>> -
>> /* save user friendly CRTC name for later */
>> snprintf(dpu_crtc->name, DPU_CRTC_NAME_SIZE, "crtc%u",
>> crtc->base.id);
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index 539b68b..8bac395 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -300,4 +300,10 @@ static inline enum dpu_crtc_client_type
>dpu_crtc_get_client_type(
>> return crtc && crtc->state ? RT_CLIENT : NRT_CLIENT;
>> }
>>
>> +/**
>> + * dpu_crtc_has_color_enabled - check if the crtc has color
>> +management enabled
>> + * @crtc: Pointer to drm crtc object
>> + */
>> +bool dpu_crtc_has_color_enabled(struct drm_crtc *crtc);
>> +
>> #endif /* _DPU_CRTC_H_ */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 4c56a16..ebc3f25 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder
>*drm_enc)
>> static struct msm_display_topology dpu_encoder_get_topology(
>> struct dpu_encoder_virt *dpu_enc,
>> struct dpu_kms *dpu_kms,
>> - struct drm_display_mode *mode)
>> + struct drm_display_mode *mode,
>> + struct drm_crtc *crtc)
>> {
>> struct msm_display_topology topology = {0};
>> int i, intf_count = 0;
>> @@ -573,11 +574,9 @@ static struct msm_display_topology
>dpu_encoder_get_topology(
>> else
>> topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT)
>> ? 2 : 1;
>>
>> - if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>> - if (dpu_kms->catalog->dspp &&
>> - (dpu_kms->catalog->dspp_count >= topology.num_lm))
>> + if (dpu_crtc_has_color_enabled(crtc) &&
>> + (dpu_kms->catalog->dspp_count >= topology.num_lm))
>
>See the comment to the previous patch. It still applies here.
>
This function will only populate the requirements for a given topology based on
mode. If there are not enough
Re: [Freedreno] [PATCH v1 6/9] arm64: dts: qcom: sm8350: Use 2 interconnect cells
On Fri, 28 Oct 2022 at 15:44, Bjorn Andersson wrote:
>
> On Fri, Oct 28, 2022 at 02:08:09PM +0200, Robert Foss wrote:
> > Use two interconnect cells in order to optionally
> > support a path tag.
> >
> > Signed-off-by: Robert Foss
> > ---
> > arch/arm64/boot/dts/qcom/sm8350.dtsi | 20 ++--
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > index 606fab087945..b6e44cd3b394 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
> > @@ -1543,56 +1543,56 @@ apps_smmu: iommu@1500 {
> > config_noc: interconnect@150 {
> > compatible = "qcom,sm8350-config-noc";
> > reg = <0 0x0150 0 0xa580>;
> > - #interconnect-cells = <1>;
> > + #interconnect-cells = <2>;
>
> You also need amend all the interconnects references with the additional
> tag cell.
Ack
>
> Regards,
> Bjorn
>
> > qcom,bcm-voters = <_bcm_voter>;
> > };
> >
> > mc_virt: interconnect@158 {
> > compatible = "qcom,sm8350-mc-virt";
> > reg = <0 0x0158 0 0x1000>;
> > - #interconnect-cells = <1>;
> > + #interconnect-cells = <2>;
> > qcom,bcm-voters = <_bcm_voter>;
> > };
> >
> > system_noc: interconnect@168 {
> > compatible = "qcom,sm8350-system-noc";
> > reg = <0 0x0168 0 0x1c200>;
> > - #interconnect-cells = <1>;
> > + #interconnect-cells = <2>;
> > qcom,bcm-voters = <_bcm_voter>;
> > };
> >
> > aggre1_noc: interconnect@16e {
> > compatible = "qcom,sm8350-aggre1-noc";
> > reg = <0 0x016e 0 0x1f180>;
> > - #interconnect-cells = <1>;
> > + #interconnect-cells = <2>;
> > qcom,bcm-voters = <_bcm_voter>;
> > };
> >
> > aggre2_noc: interconnect@170 {
> > compatible = "qcom,sm8350-aggre2-noc";
> > reg = <0 0x0170 0 0x33000>;
> > - #interconnect-cells = <1>;
> > + #interconnect-cells = <2>;
> > qcom,bcm-voters = <_bcm_voter>;
> > };
> >
> > mmss_noc: interconnect@174 {
> > compatible = "qcom,sm8350-mmss-noc";
> > reg = <0 0x0174 0 0x1f080>;
> > - #interconnect-cells = <1>;
> > + #interconnect-cells = <2>;
> > qcom,bcm-voters = <_bcm_voter>;
> > };
> >
> > lpass_ag_noc: interconnect@3c4 {
> > compatible = "qcom,sm8350-lpass-ag-noc";
> > reg = <0 0x03c4 0 0xf080>;
> > - #interconnect-cells = <1>;
> > + #interconnect-cells = <2>;
> > qcom,bcm-voters = <_bcm_voter>;
> > };
> >
> > compute_noc: interconnect@a0c{
> > compatible = "qcom,sm8350-compute-noc";
> > reg = <0 0x0a0c 0 0xa180>;
> > - #interconnect-cells = <1>;
> > + #interconnect-cells = <2>;
> > qcom,bcm-voters = <_bcm_voter>;
> > };
> >
> > @@ -2420,14 +2420,14 @@ usb_2_ssphy: phy@88ebe00 {
> > dc_noc: interconnect@90c {
> > compatible = "qcom,sm8350-dc-noc";
> > reg = <0 0x090c 0 0x4200>;
> > - #interconnect-cells = <1>;
> > + #interconnect-cells = <2>;
> > qcom,bcm-voters = <_bcm_voter>;
> > };
> >
> > gem_noc: interconnect@910 {
> > compatible = "qcom,sm8350-gem-noc";
> > reg = <0 0x0910 0 0xb4000>;
> > - #interconnect-cells = <1>;
> > + #interconnect-cells = <2>;
> > qcom,bcm-voters = <_bcm_voter>;
> > };
> >
> > --
> > 2.34.1
> >
Re: [Freedreno] [PATCH v1 3/9] drm/msm/dpu: Add SM8350 to hw catalog
On Fri, 28 Oct 2022 at 14:27, Dmitry Baryshkov
wrote:
>
> On 28/10/2022 15:08, Robert Foss wrote:
> > Add compatibility for SM8350 display subsystem, including
> > required entries in DPU hw catalog.
> > ---
> > .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 217 ++
> > .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 1 +
> > 2 files changed, 218 insertions(+)
> >
> > 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 d0ce7612fee8..bc829d7bdd6e 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > @@ -112,6 +112,15 @@
> >BIT(MDP_INTF3_INTR) | \
> >BIT(MDP_INTF4_INTR))
> >
> > +#define IRQ_SM8350_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
> > + BIT(MDP_SSPP_TOP0_INTR2) | \
> > + BIT(MDP_SSPP_TOP0_HIST_INTR) | \
> > + BIT(MDP_INTF0_7xxx_INTR) | \
> > + BIT(MDP_INTF1_7xxx_INTR) | \
> > + BIT(MDP_INTF2_7xxx_INTR) | \
> > + BIT(MDP_INTF3_7xxx_INTR) | \
> > + 0)
> > +
> > #define IRQ_SC8180X_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
> > BIT(MDP_SSPP_TOP0_INTR2) | \
> > BIT(MDP_SSPP_TOP0_HIST_INTR) | \
> > @@ -364,6 +373,20 @@ static const struct dpu_caps sm8250_dpu_caps = {
> > .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > };
> >
> > +static const struct dpu_caps sm8350_dpu_caps = {
> > + .max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
> > + .max_mixer_blendstages = 0xb,
> > + .qseed_type = DPU_SSPP_SCALER_QSEED3LITE,
> > + .smart_dma_rev = DPU_SSPP_SMART_DMA_V2, /* TODO: v2.5 */
> > + .ubwc_version = DPU_HW_UBWC_VER_40,
> > + .has_src_split = true,
> > + .has_dim_layer = true,
> > + .has_idle_pc = true,
> > + .has_3d_merge = true,
> > + .max_linewidth = 4096,
>
> Is it 4096 or 5120?
4096 is what I'm seeing in the downstream dts, except for the
wb-linewidth-linear property which is 5120.
So I would think 4096 is the correct value, what do you think?
>
> > + .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> > +};
> > +
> > static const struct dpu_caps sc7280_dpu_caps = {
> > .max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
> > .max_mixer_blendstages = 0x7,
> > @@ -501,6 +524,33 @@ static const struct dpu_mdp_cfg sm8250_mdp[] = {
> > },
> > };
> >
> > +static const struct dpu_mdp_cfg sm8350_mdp[] = {
> > + {
> > + .name = "top_0", .id = MDP_TOP,
> > + .base = 0x0, .len = 0x494,
> > + .features = 0,
> > + .highest_bank_bit = 0x3, /* TODO: 2 for LP_DDR4 */
> > + .clk_ctrls[DPU_CLK_CTRL_VIG0] = {
> > + .reg_off = 0x2AC, .bit_off = 0},
> > + .clk_ctrls[DPU_CLK_CTRL_VIG1] = {
> > + .reg_off = 0x2B4, .bit_off = 0},
> > + .clk_ctrls[DPU_CLK_CTRL_VIG2] = {
> > + .reg_off = 0x2BC, .bit_off = 0},
> > + .clk_ctrls[DPU_CLK_CTRL_VIG3] = {
> > + .reg_off = 0x2C4, .bit_off = 0},
> > + .clk_ctrls[DPU_CLK_CTRL_DMA0] = {
> > + .reg_off = 0x2AC, .bit_off = 8},
> > + .clk_ctrls[DPU_CLK_CTRL_DMA1] = {
> > + .reg_off = 0x2B4, .bit_off = 8},
> > + .clk_ctrls[DPU_CLK_CTRL_CURSOR0] = {
> > + .reg_off = 0x2BC, .bit_off = 8},
> > + .clk_ctrls[DPU_CLK_CTRL_CURSOR1] = {
> > + .reg_off = 0x2C4, .bit_off = 8},
> > + .clk_ctrls[DPU_CLK_CTRL_REG_DMA] = {
> > + .reg_off = 0x2BC, .bit_off = 20},
> > + },
> > +};
> > +
> > static const struct dpu_mdp_cfg sc7280_mdp[] = {
> > {
> > .name = "top_0", .id = MDP_TOP,
> > @@ -659,6 +709,66 @@ static const struct dpu_ctl_cfg sm8150_ctl[] = {
> > },
> > };
> >
> > +static const struct dpu_ctl_cfg sm8350_ctl[] = {
> > + {
> > + .name = "ctl_0", .id = CTL_0,
> > + .base = 0x15000, .len = 0x1e8,
> > + .features = BIT(DPU_CTL_SPLIT_DISPLAY) |
> > + BIT(DPU_CTL_PINGPONG_SPLIT) |
> > + BIT(DPU_CTL_ACTIVE_CFG) |
> > + BIT(DPU_CTL_FETCH_ACTIVE) |
> > + BIT(DPU_CTL_VM_CFG) |
> > + BIT(DPU_CTL_UNIFIED_DSPP_FLUSH),
> > + .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9),
> > + },
> > + {
> > + .name = "ctl_1", .id = CTL_1,
> > + .base = 0x16000, .len = 0x1e8,
> > + .features = BIT(DPU_CTL_SPLIT_DISPLAY) |
> > + BIT(DPU_CTL_ACTIVE_CFG) |
> > + BIT(DPU_CTL_FETCH_ACTIVE) |
> > + BIT(DPU_CTL_VM_CFG) |
> > + BIT(DPU_CTL_UNIFIED_DSPP_FLUSH),
>
> The UNIFIED_DSPP_FLUSH is not merged. Could you please change this to
> BIT(DPU_CTL_SPLIT_DISPLAY) | CTL_SC7280_MASK (for first two CTLs) and
> just CTL_SC7280_MASK for the rest of
Re: [Freedreno] [PATCH 3/5] drm/msm: Fix IS_ERR_OR_NULL() vs NULL check in msm_icc_get()
On 2022-11-10 17:44:43, Gaosheng Cui wrote:
> The of_icc_get() function returns NULL or error pointers on error path,
> so we should use IS_ERR_OR_NULL() to check the return value.
>
> Fixes: 5ccdcecaf8f7 ("drm/msm: lookup the ICC paths in both mdp5/dpu and mdss
> devices")
> Signed-off-by: Gaosheng Cui
> ---
> drivers/gpu/drm/msm/msm_io_utils.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_io_utils.c
> b/drivers/gpu/drm/msm/msm_io_utils.c
> index d02cd29ce829..950083b2f092 100644
> --- a/drivers/gpu/drm/msm/msm_io_utils.c
> +++ b/drivers/gpu/drm/msm/msm_io_utils.c
> @@ -133,7 +133,7 @@ struct icc_path *msm_icc_get(struct device *dev, const
> char *name)
> struct icc_path *path;
>
> path = of_icc_get(dev, name);
> - if (path)
> + if (IS_ERR_OR_NULL(path))
NAK. This path should be returned if it is NON-NULL, otherwise we defer
to of_icc_get() on the parent device. See the code-comment below.
- Marijn
> return path;
>
> /*
> --
> 2.25.1
>
Re: [Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane
On Wed, Nov 09, 2022 at 05:44:37PM -0800, Jessica Zhang wrote: > > > On 11/9/2022 5:59 AM, Daniel Vetter wrote: > > On Wed, Nov 09, 2022 at 04:53:45PM +0300, Dmitry Baryshkov wrote: > > > On 09/11/2022 16:52, Daniel Vetter wrote: > > > > On Tue, Nov 08, 2022 at 06:25:29PM +, Simon Ser wrote: > > > > > On Saturday, October 29th, 2022 at 13:23, Dmitry Baryshkov > > > > > wrote: > > > > > > > > > > > On 29/10/2022 01:59, Jessica Zhang wrote: > > > > > > > > > > > > > Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for > > > > > > > drm_plane. In addition, add support for setting and getting the > > > > > > > values > > > > > > > of these properties. > > > > > > > > > > > > > > COLOR_FILL represents the color fill of a plane while > > > > > > > COLOR_FILL_FORMAT > > > > > > > represents the format of the color fill. Userspace can set enable > > > > > > > solid > > > > > > > fill on a plane by assigning COLOR_FILL to a uint64_t value, > > > > > > > assigning > > > > > > > the COLOR_FILL_FORMAT property to a uint32_t value, and setting > > > > > > > the > > > > > > > framebuffer to NULL. > > > > > > > > > > > > I suppose that COLOR_FILL should override framebuffer rather than > > > > > > requiring that FB is set to NULL. In other words, if > > > > > > color_filL_format > > > > > > is non-zero, it would make sense to ignore the FB. Then one can use > > > > > > the > > > > > > color_fill_format property to quickly switch between filled plane > > > > > > and > > > > > > FB-backed one. > > > > > > > > > > That would be inconsistent with the rest of the KMS uAPI. For > > > > > instance, > > > > > the kernel will error out if CRTC has active=0 but a connector is > > > > > still > > > > > linked to the CRTC. IOW, the current uAPI errors out if the KMS state > > > > > is inconsistent. > > > > > > > > So if the use-case here really is to solid-fill a plane (and not just > > > > provide a background color for the crtc overall), then I guess we could > > > > also extend addfb to make that happen. We've talked in the past about > > > > propertery-fying framebuffer objects, and that would sort out this uapi > > > > wart. And I agree the color fill vs PLANE_ID issue is a bit ugly at > > > > least. > > > > > > > > But if the use-cases are all background color then just doing the crtc > > > > background color would be tons simpler (and likely also easier to > > > > support > > > > for more hardware). > > > > > > No. The hardware supports multiple color-filled planes, which do not have > > > to > > > cover the whole CRTC. > > > > The use case here means the userspace use-case. What the hw can do on any > > given chip kinda doesnt matter, which is why I'm asking. KMD uapi is not > > meant to reflect 100% exactly what a specific chip can do, but instead: > > - provide features userspace actually needs. If you want per-plane fill, > >you need userspace that makes use of per-plane fill, and if all you have > >is crtc background, then that's it. > > Hey Daniel, > > The userspace use case we're trying to support is the Android HWC SOLID_FILL > hint here [1], which is specifying per-plane fill. Does surfaceflinger actually use this for more than background fills? Yes I'm annoying, but if we can simplify the kernel driver implementation burden by asking compositors to do the math and simplify things, then I think we should. We also need an open source implementation for this that works and is tested end-to-end. There's the drm_hwc project, but last time I've checked there's really not much happpening there unfortunately. -Daniel > > Thanks, > > Jessica Zhang > > [1] > https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/graphics/composer/aidl/android/hardware/graphics/composer3/Composition.aidl#52 > > > - we should create uapi with an eye towards what's actually possible on a > >reasonable set of drivers and hw. Sometimes that means a slightly more > >restricted set so that it's possible to implement in more places, > >especially if that restricted feature set still gets the job done for > >userspace. > > > > Cheers, Daniel > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
