Re: [Freedreno] [PATCH] drm/msm/dpu: Fix encoder CRC to account for CTM enablement
On 11/21/2023 4:27 PM, Rob Clark wrote:
On Tue, Nov 21, 2023 at 4:41 PM Abhinav Kumar wrote:
On 10/24/2023 12:01 PM, Abhinav Kumar wrote:
On 10/23/2023 4:03 PM, Dmitry Baryshkov wrote:
On Tue, 24 Oct 2023 at 01:36, Rob Clark wrote:
On Mon, Oct 23, 2023 at 3:30 PM Dmitry Baryshkov
wrote:
On Tue, 24 Oct 2023 at 01:12, Rob Clark wrote:
From: Rob Clark
Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not
otherwise.
Suggested-by: Dmitry Baryshkov
Signed-off-by: Rob Clark
---
I cannot find anything in the docs which suggest this solution is correct.
Different blocks in the DPU pipeline have their own CRC (MISR) registers
like LM, intf etc.
We dont need to change INPUT_SEL to tell DPU from which pipeline to take
the CRC from as each of them have their own registers.
INPUT_SEL is controlling whether the CRC needs to be calculated over the
entire display timings or only the active pixels. I am unable to tell at
the moment why this is making a difference in this use-case.
Since I am unable to find any documentation proving this solution is
correct so far, unfortunately I would hold this back till then.
We will investigate this issue and report our findings on this thread on
how to proceed.
Alright, we debugged and also found some more answers.
The correct solution is indeed to set INPUT_SEL = 1 but let me explain
why and what should be the correct way.
INPUT_SEL was indeed telling whether to compute CRC over active pixels
or active pixels + timings like I wrote before but this behavior changed
since some chipsets.
Now, INPUT_SEL = 0 means compute CRC *only* over timings and not the
active area (and not display + timings like before) and like mentioned
before this has nothing to do with what is the input to the CRC. Not
covering the active area will not change the CRC at all as Rob reported
but its not specific to CTM.
Which means we should have been setting INPUT_SEL=1 whenever we use INTF
CRC irrespective of whether CTM is used or not.
What this also means is INTF CRC was not working correctly at all so far
irrespecive of CTM or not because it was always computing CRC only on
the timings (non-active area).
This was not caught so far because it looks like IGT's
kms_pipe_crc_basic test which was used to validate this only compares
CRC between two frames of the same content to match if they were equal
and not changing contents and comparing like kms_plane does. It will
pass as CRC would not have changed.
Now coming to the fix, the reset value of this register INTF_MISR_CTRL
already sets the INPUT_SEL bit (or unsets it) correctly based on
whichever DPU version is used so we should just change the
dpu_hw_setup_misr() to a read on the register followed by ORing the
required bits without touching INPUT_SEL and write.
That will address this issue and also cover version control since the
expected value of this bit has changed across DPU revisions.
Ok, thanks for following up on this. Mind posting a patch to
supersede this one?
BR,
-R
Yup, we will.
Thanks
Abhinav
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++-
8 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 2b83a13b3aa9..d93a92ffd5df 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct
drm_crtc *crtc)
struct drm_encoder *drm_enc;
drm_for_each_encoder_mask(drm_enc, crtc->dev,
crtc->state->encoder_mask)
- dpu_encoder_setup_misr(drm_enc);
+ dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm);
}
static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const
char *src_name)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b0a7908418ed..12ee7acb5ea6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct
drm_encoder *drm_enc)
return num_intf;
}
-void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc)
+void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc,
bool has_ctm)
{
struct dpu_encoder_virt *dpu_enc;
@@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct
drm_encoder *drm_enc)
if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr)
continue;
-
Re: [Freedreno] [PATCH] drm/msm/dpu: Fix encoder CRC to account for CTM enablement
On Tue, Nov 21, 2023 at 4:41 PM Abhinav Kumar wrote: > > > > On 10/24/2023 12:01 PM, Abhinav Kumar wrote: > > > > > > On 10/23/2023 4:03 PM, Dmitry Baryshkov wrote: > >> On Tue, 24 Oct 2023 at 01:36, Rob Clark wrote: > >>> > >>> On Mon, Oct 23, 2023 at 3:30 PM Dmitry Baryshkov > >>> wrote: > > On Tue, 24 Oct 2023 at 01:12, Rob Clark wrote: > > > > From: Rob Clark > > > > Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not > > otherwise. > > > > Suggested-by: Dmitry Baryshkov > > Signed-off-by: Rob Clark > > --- > > > > I cannot find anything in the docs which suggest this solution is correct. > > > > Different blocks in the DPU pipeline have their own CRC (MISR) registers > > like LM, intf etc. > > > > We dont need to change INPUT_SEL to tell DPU from which pipeline to take > > the CRC from as each of them have their own registers. > > > > INPUT_SEL is controlling whether the CRC needs to be calculated over the > > entire display timings or only the active pixels. I am unable to tell at > > the moment why this is making a difference in this use-case. > > > > Since I am unable to find any documentation proving this solution is > > correct so far, unfortunately I would hold this back till then. > > > > We will investigate this issue and report our findings on this thread on > > how to proceed. > > > > Alright, we debugged and also found some more answers. > > The correct solution is indeed to set INPUT_SEL = 1 but let me explain > why and what should be the correct way. > > INPUT_SEL was indeed telling whether to compute CRC over active pixels > or active pixels + timings like I wrote before but this behavior changed > since some chipsets. > > Now, INPUT_SEL = 0 means compute CRC *only* over timings and not the > active area (and not display + timings like before) and like mentioned > before this has nothing to do with what is the input to the CRC. Not > covering the active area will not change the CRC at all as Rob reported > but its not specific to CTM. > > Which means we should have been setting INPUT_SEL=1 whenever we use INTF > CRC irrespective of whether CTM is used or not. > > What this also means is INTF CRC was not working correctly at all so far > irrespecive of CTM or not because it was always computing CRC only on > the timings (non-active area). > > This was not caught so far because it looks like IGT's > kms_pipe_crc_basic test which was used to validate this only compares > CRC between two frames of the same content to match if they were equal > and not changing contents and comparing like kms_plane does. It will > pass as CRC would not have changed. > > Now coming to the fix, the reset value of this register INTF_MISR_CTRL > already sets the INPUT_SEL bit (or unsets it) correctly based on > whichever DPU version is used so we should just change the > dpu_hw_setup_misr() to a read on the register followed by ORing the > required bits without touching INPUT_SEL and write. > > That will address this issue and also cover version control since the > expected value of this bit has changed across DPU revisions. Ok, thanks for following up on this. Mind posting a patch to supersede this one? BR, -R > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 2 +- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++-- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++-- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 - > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++- > > 8 files changed, 15 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index 2b83a13b3aa9..d93a92ffd5df 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct > > drm_crtc *crtc) > > struct drm_encoder *drm_enc; > > > > drm_for_each_encoder_mask(drm_enc, crtc->dev, > > crtc->state->encoder_mask) > > - dpu_encoder_setup_misr(drm_enc); > > + dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm); > > } > > > > static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const > > char *src_name) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index b0a7908418ed..12ee7acb5ea6 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct > > drm_encoder *drm_enc) > > return num_intf;
Re: [Freedreno] [PATCH] drm/msm/dpu: Fix encoder CRC to account for CTM enablement
On 10/24/2023 12:01 PM, Abhinav Kumar wrote:
On 10/23/2023 4:03 PM, Dmitry Baryshkov wrote:
On Tue, 24 Oct 2023 at 01:36, Rob Clark wrote:
On Mon, Oct 23, 2023 at 3:30 PM Dmitry Baryshkov
wrote:
On Tue, 24 Oct 2023 at 01:12, Rob Clark wrote:
From: Rob Clark
Seems like we need to pick INPUT_SEL=1 when CTM is enabled. But not
otherwise.
Suggested-by: Dmitry Baryshkov
Signed-off-by: Rob Clark
---
I cannot find anything in the docs which suggest this solution is correct.
Different blocks in the DPU pipeline have their own CRC (MISR) registers
like LM, intf etc.
We dont need to change INPUT_SEL to tell DPU from which pipeline to take
the CRC from as each of them have their own registers.
INPUT_SEL is controlling whether the CRC needs to be calculated over the
entire display timings or only the active pixels. I am unable to tell at
the moment why this is making a difference in this use-case.
Since I am unable to find any documentation proving this solution is
correct so far, unfortunately I would hold this back till then.
We will investigate this issue and report our findings on this thread on
how to proceed.
Alright, we debugged and also found some more answers.
The correct solution is indeed to set INPUT_SEL = 1 but let me explain
why and what should be the correct way.
INPUT_SEL was indeed telling whether to compute CRC over active pixels
or active pixels + timings like I wrote before but this behavior changed
since some chipsets.
Now, INPUT_SEL = 0 means compute CRC *only* over timings and not the
active area (and not display + timings like before) and like mentioned
before this has nothing to do with what is the input to the CRC. Not
covering the active area will not change the CRC at all as Rob reported
but its not specific to CTM.
Which means we should have been setting INPUT_SEL=1 whenever we use INTF
CRC irrespective of whether CTM is used or not.
What this also means is INTF CRC was not working correctly at all so far
irrespecive of CTM or not because it was always computing CRC only on
the timings (non-active area).
This was not caught so far because it looks like IGT's
kms_pipe_crc_basic test which was used to validate this only compares
CRC between two frames of the same content to match if they were equal
and not changing contents and comparing like kms_plane does. It will
pass as CRC would not have changed.
Now coming to the fix, the reset value of this register INTF_MISR_CTRL
already sets the INPUT_SEL bit (or unsets it) correctly based on
whichever DPU version is used so we should just change the
dpu_hw_setup_misr() to a read on the register followed by ORing the
required bits without touching INPUT_SEL and write.
That will address this issue and also cover version control since the
expected value of this bit has changed across DPU revisions.
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.c | 5 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_util.h | 3 ++-
8 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 2b83a13b3aa9..d93a92ffd5df 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -134,7 +134,7 @@ static void dpu_crtc_setup_encoder_misr(struct
drm_crtc *crtc)
struct drm_encoder *drm_enc;
drm_for_each_encoder_mask(drm_enc, crtc->dev,
crtc->state->encoder_mask)
- dpu_encoder_setup_misr(drm_enc);
+ dpu_encoder_setup_misr(drm_enc, !!crtc->state->ctm);
}
static int dpu_crtc_set_crc_source(struct drm_crtc *crtc, const
char *src_name)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b0a7908418ed..12ee7acb5ea6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -241,7 +241,7 @@ int dpu_encoder_get_crc_values_cnt(const struct
drm_encoder *drm_enc)
return num_intf;
}
-void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc)
+void dpu_encoder_setup_misr(const struct drm_encoder *drm_enc,
bool has_ctm)
{
struct dpu_encoder_virt *dpu_enc;
@@ -255,7 +255,7 @@ void dpu_encoder_setup_misr(const struct
drm_encoder *drm_enc)
if (!phys->hw_intf || !phys->hw_intf->ops.setup_misr)
continue;
- phys->hw_intf->ops.setup_misr(phys->hw_intf, true, 1);
+ phys->hw_intf->ops.setup_misr(phys->hw_intf, true,
1, has_ctm);
}
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
Re: [Freedreno] [PATCH v2 2/2] drm/msm/dp: attach the DP subconnector property
On 21.11.23 19:50, Abhinav Kumar wrote:
> On 11/21/2023 9:57 AM, Linux regression tracking (Thorsten Leemhuis) wrote:
>> On 15.11.23 19:06, Abhinav Kumar wrote:
>>> On 11/15/2023 12:06 AM, Johan Hovold wrote:
On Wed, Oct 25, 2023 at 12:23:10PM +0300, Dmitry Baryshkov wrote:
> While developing and testing the commit bfcc3d8f94f4 ("drm/msm/dp:
> support setting the DP subconnector type") I had the patch [1] in my
> tree. I haven't noticed that it was a dependency for the commit in
> question. Mea culpa.
This also broke boot on the Lenovo ThinkPad X13s.
Would be nice to get this fixed ASAP so that further people don't have
to debug this known regression.
>>>
>>> I will queue this patch for -fixes rightaway.
>>
>> Thx. I noticed that this fix is still not in -next. I then investigated
>> and I found it was applied on Thursday last week here:
>> https://gitlab.freedesktop.org/drm/msm/-/commits/msm-fixes?ref_type=heads
>>
>> Makes me wonder: when will that patch go to a branch that is included in
>> -next? And when will it move on towards mainline?
>
> This has been included in a pull request for 6.7-rc3 to the DRM tree and
> shall make it to -next from there.
Ahh, great, thx, I was slowly getting worried.
Ciao, Thorsten
> Since the patch has not landed yet (and even was not reviewed)
> and since one of the bridges erroneously uses USB connector type
> instead
> of DP, attach the property directly from the MSM DP driver.
>
> This fixes the following oops on DP HPD event:
>
> drm_object_property_set_value
> (drivers/gpu/drm/drm_mode_object.c:288)
> dp_display_process_hpd_high
> (drivers/gpu/drm/msm/dp/dp_display.c:402)
> dp_hpd_plug_handle.isra.0 (drivers/gpu/drm/msm/dp/dp_display.c:604)
> hpd_event_thread (drivers/gpu/drm/msm/dp/dp_display.c:1110)
> kthread (kernel/kthread.c:388)
> ret_from_fork (arch/arm64/kernel/entry.S:858)
This only says where the oops happened, it doesn't necessarily in
itself
indicate an oops at all or that in this case it's a NULL pointer
dereference.
On the X13s I'm seeing the NULL deref in a different path during boot,
and when this happens after a deferred probe (due to the panel lookup
mess) it hangs the machine, which makes it a bit of a pain to debug:
Unable to handle kernel NULL pointer dereference at virtual
address 0060
...
CPU: 4 PID: 57 Comm: kworker/u16:1 Not tainted 6.7.0-rc1 #4
Hardware name: Qualcomm QRD, BIOS
6.0.220110.BOOT.MXF.1.1-00470-MAKENA-1 01/10/2022
...
Call trace:
drm_object_property_set_value+0x0/0x88 [drm]
dp_display_process_hpd_high+0xa0/0x14c [msm]
dp_hpd_plug_handle.constprop.0.isra.0+0x90/0x110 [msm]
dp_bridge_atomic_enable+0x184/0x21c [msm]
edp_bridge_atomic_enable+0x60/0x94 [msm]
drm_atomic_bridge_chain_enable+0x54/0xc8 [drm]
drm_atomic_helper_commit_modeset_enables+0x194/0x26c
[drm_kms_helper]
msm_atomic_commit_tail+0x204/0x804 [msm]
commit_tail+0xa4/0x18c [drm_kms_helper]
drm_atomic_helper_commit+0x19c/0x1b0 [drm_kms_helper]
drm_atomic_commit+0xa4/0x104 [drm]
drm_client_modeset_commit_atomic+0x22c/0x298 [drm]
drm_client_modeset_commit_locked+0x60/0x1c0 [drm]
drm_client_modeset_commit+0x30/0x58 [drm]
__drm_fb_helper_restore_fbdev_mode_unlocked+0xbc/0xfc
[drm_kms_helper]
drm_fb_helper_set_par+0x30/0x4c [drm_kms_helper]
fbcon_init+0x224/0x49c
visual_init+0xb0/0x108
do_bind_con_driver.isra.0+0x19c/0x38c
do_take_over_console+0x140/0x1ec
do_fbcon_takeover+0x6c/0xe4
fbcon_fb_registered+0x180/0x1f0
register_framebuffer+0x19c/0x228
__drm_fb_helper_initial_config_and_unlock+0x2e8/0x4e8
[drm_kms_helper]
drm_fb_helper_initial_config+0x3c/0x4c [drm_kms_helper]
msm_fbdev_client_hotplug+0x84/0xcc [msm]
drm_client_register+0x5c/0xa0 [drm]
msm_fbdev_setup+0x94/0x148 [msm]
msm_drm_bind+0x3d0/0x42c [msm]
try_to_bring_up_aggregate_device+0x1ec/0x2f4
__component_add+0xa8/0x194
component_add+0x14/0x20
dp_display_probe+0x278/0x41c [msm]
> [1] https://patchwork.freedesktop.org/patch/30/
>
> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector
> type")
> Reviewed-by: Abhinav Kumar
> Signed-off-by: Dmitry Baryshkov
Reviewed-by: Johan Hovold
Tested-by: Johan Hovold
>>>
>>> Thanks !
>>>
Johan
>
>
Re: [Freedreno] [PATCH v2 2/2] drm/msm/dp: attach the DP subconnector property
On 11/21/2023 9:57 AM, Linux regression tracking (Thorsten Leemhuis) wrote:
On 15.11.23 19:06, Abhinav Kumar wrote:
On 11/15/2023 12:06 AM, Johan Hovold wrote:
On Wed, Oct 25, 2023 at 12:23:10PM +0300, Dmitry Baryshkov wrote:
While developing and testing the commit bfcc3d8f94f4 ("drm/msm/dp:
support setting the DP subconnector type") I had the patch [1] in my
tree. I haven't noticed that it was a dependency for the commit in
question. Mea culpa.
This also broke boot on the Lenovo ThinkPad X13s.
Would be nice to get this fixed ASAP so that further people don't have
to debug this known regression.
I will queue this patch for -fixes rightaway.
Thx. I noticed that this fix is still not in -next. I then investigated
and I found it was applied on Thursday last week here:
https://gitlab.freedesktop.org/drm/msm/-/commits/msm-fixes?ref_type=heads
Makes me wonder: when will that patch go to a branch that is included in
-next? And when will it move on towards mainline?
This has been included in a pull request for 6.7-rc3 to the DRM tree and
shall make it to -next from there.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
Since the patch has not landed yet (and even was not reviewed)
and since one of the bridges erroneously uses USB connector type instead
of DP, attach the property directly from the MSM DP driver.
This fixes the following oops on DP HPD event:
drm_object_property_set_value (drivers/gpu/drm/drm_mode_object.c:288)
dp_display_process_hpd_high (drivers/gpu/drm/msm/dp/dp_display.c:402)
dp_hpd_plug_handle.isra.0 (drivers/gpu/drm/msm/dp/dp_display.c:604)
hpd_event_thread (drivers/gpu/drm/msm/dp/dp_display.c:1110)
kthread (kernel/kthread.c:388)
ret_from_fork (arch/arm64/kernel/entry.S:858)
This only says where the oops happened, it doesn't necessarily in itself
indicate an oops at all or that in this case it's a NULL pointer
dereference.
On the X13s I'm seeing the NULL deref in a different path during boot,
and when this happens after a deferred probe (due to the panel lookup
mess) it hangs the machine, which makes it a bit of a pain to debug:
Unable to handle kernel NULL pointer dereference at virtual
address 0060
...
CPU: 4 PID: 57 Comm: kworker/u16:1 Not tainted 6.7.0-rc1 #4
Hardware name: Qualcomm QRD, BIOS
6.0.220110.BOOT.MXF.1.1-00470-MAKENA-1 01/10/2022
...
Call trace:
drm_object_property_set_value+0x0/0x88 [drm]
dp_display_process_hpd_high+0xa0/0x14c [msm]
dp_hpd_plug_handle.constprop.0.isra.0+0x90/0x110 [msm]
dp_bridge_atomic_enable+0x184/0x21c [msm]
edp_bridge_atomic_enable+0x60/0x94 [msm]
drm_atomic_bridge_chain_enable+0x54/0xc8 [drm]
drm_atomic_helper_commit_modeset_enables+0x194/0x26c
[drm_kms_helper]
msm_atomic_commit_tail+0x204/0x804 [msm]
commit_tail+0xa4/0x18c [drm_kms_helper]
drm_atomic_helper_commit+0x19c/0x1b0 [drm_kms_helper]
drm_atomic_commit+0xa4/0x104 [drm]
drm_client_modeset_commit_atomic+0x22c/0x298 [drm]
drm_client_modeset_commit_locked+0x60/0x1c0 [drm]
drm_client_modeset_commit+0x30/0x58 [drm]
__drm_fb_helper_restore_fbdev_mode_unlocked+0xbc/0xfc
[drm_kms_helper]
drm_fb_helper_set_par+0x30/0x4c [drm_kms_helper]
fbcon_init+0x224/0x49c
visual_init+0xb0/0x108
do_bind_con_driver.isra.0+0x19c/0x38c
do_take_over_console+0x140/0x1ec
do_fbcon_takeover+0x6c/0xe4
fbcon_fb_registered+0x180/0x1f0
register_framebuffer+0x19c/0x228
__drm_fb_helper_initial_config_and_unlock+0x2e8/0x4e8
[drm_kms_helper]
drm_fb_helper_initial_config+0x3c/0x4c [drm_kms_helper]
msm_fbdev_client_hotplug+0x84/0xcc [msm]
drm_client_register+0x5c/0xa0 [drm]
msm_fbdev_setup+0x94/0x148 [msm]
msm_drm_bind+0x3d0/0x42c [msm]
try_to_bring_up_aggregate_device+0x1ec/0x2f4
__component_add+0xa8/0x194
component_add+0x14/0x20
dp_display_probe+0x278/0x41c [msm]
[1] https://patchwork.freedesktop.org/patch/30/
Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector
type")
Reviewed-by: Abhinav Kumar
Signed-off-by: Dmitry Baryshkov
Reviewed-by: Johan Hovold
Tested-by: Johan Hovold
Thanks !
Johan
Re: [Freedreno] [PATCH v2] drm/msm: remove unnecessary NULL check
On Fri, 13 Oct 2023 11:25:15 +0300, Dan Carpenter wrote: > This NULL check was required when it was added, but we shuffled the code > around and now it's not. The inconsistent NULL checking triggers a > Smatch warning: > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn: > variable dereferenced before check 'mdp5_kms' (see line 782) > > [...] Applied, thanks! [1/1] drm/msm: remove unnecessary NULL check https://gitlab.freedesktop.org/drm/msm/-/commit/56466f653cb5 Best regards, -- Abhinav Kumar
Re: [Freedreno] [PATCH v2 0/2] drm/msm/dp: fix DP subconnector handling
On Wed, 25 Oct 2023 12:23:08 +0300, Dmitry Baryshkov wrote: > Fix two failovers in the DP subconnector's patch. I didn't notice that I > had another patch adding the property in my tree and later Abel pointed > out that we shouldn't use subconnector type for eDP panels. > > Fixes since v1: > - Add Abel's patch. > > [...] Applied, thanks! [1/2] drm/msm/dp: don't touch DP subconnector property in eDP case https://gitlab.freedesktop.org/drm/msm/-/commit/ebfa85c504cb [2/2] drm/msm/dp: attach the DP subconnector property https://gitlab.freedesktop.org/drm/msm/-/commit/21133266ca12 Best regards, -- Abhinav Kumar
Re: [Freedreno] [PATCH] drm/msm/dpu: Add missing safe_lut_tbl in sc8280xp catalog
On Mon, 30 Oct 2023 16:23:20 -0700, Bjorn Andersson wrote: > During USB transfers on the SC8280XP __arm_smmu_tlb_sync() is seen to > typically take 1-2ms to complete. As expected this results in poor > performance, something that has been mitigated by proposing running the > iommu in non-strict mode (boot with iommu.strict=0). > > This turns out to be related to the SAFE logic, and programming the QOS > SAFE values in the DPU (per suggestion from Rob and Doug) reduces the > TLB sync time to below 10us, which means significant less time spent > with interrupts disabled and a significant boost in throughput. > > [...] Applied, thanks! [1/1] drm/msm/dpu: Add missing safe_lut_tbl in sc8280xp catalog https://gitlab.freedesktop.org/drm/msm/-/commit/a33b2431d11b Best regards, -- Abhinav Kumar
Re: [Freedreno] [PATCH] drm/msm: remove exra drm_kms_helper_poll_init() call
On Tue, 07 Nov 2023 13:14:13 +0200, Dmitry Baryshkov wrote: > It seems during rebases I have left a call to drm_kms_helper_poll_init() > which is not guarded by the (priv->kms_init) check. This leads to the > crash for the boards which don't have KMS output. Drop this call, as > there is a correctly guarded one next to the one being removed. > > Applied, thanks! [1/1] drm/msm: remove exra drm_kms_helper_poll_init() call https://gitlab.freedesktop.org/drm/msm/-/commit/3944e343e54b Best regards, -- Abhinav Kumar
Re: [Freedreno] [PATCH v2] drm/msm/dsi: use the correct VREG_CTRL_1 value for 4nm cphy
On Thu, 09 Nov 2023 19:02:14 -0500, Jonathan Marek wrote: > Use the same value as the downstream driver. This change is needed for CPHY > mode to work correctly. > > Applied, thanks! [1/1] drm/msm/dsi: use the correct VREG_CTRL_1 value for 4nm cphy https://gitlab.freedesktop.org/drm/msm/-/commit/b3e0f94d1570 Best regards, -- Abhinav Kumar
Re: [Freedreno] [PATCH v2 2/2] drm/msm/dp: attach the DP subconnector property
On 15.11.23 19:06, Abhinav Kumar wrote:
> On 11/15/2023 12:06 AM, Johan Hovold wrote:
>> On Wed, Oct 25, 2023 at 12:23:10PM +0300, Dmitry Baryshkov wrote:
>>> While developing and testing the commit bfcc3d8f94f4 ("drm/msm/dp:
>>> support setting the DP subconnector type") I had the patch [1] in my
>>> tree. I haven't noticed that it was a dependency for the commit in
>>> question. Mea culpa.
>>
>> This also broke boot on the Lenovo ThinkPad X13s.
>>
>> Would be nice to get this fixed ASAP so that further people don't have
>> to debug this known regression.
>
> I will queue this patch for -fixes rightaway.
Thx. I noticed that this fix is still not in -next. I then investigated
and I found it was applied on Thursday last week here:
https://gitlab.freedesktop.org/drm/msm/-/commits/msm-fixes?ref_type=heads
Makes me wonder: when will that patch go to a branch that is included in
-next? And when will it move on towards mainline?
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
>>> Since the patch has not landed yet (and even was not reviewed)
>>> and since one of the bridges erroneously uses USB connector type instead
>>> of DP, attach the property directly from the MSM DP driver.
>>>
>>> This fixes the following oops on DP HPD event:
>>>
>>> drm_object_property_set_value (drivers/gpu/drm/drm_mode_object.c:288)
>>> dp_display_process_hpd_high (drivers/gpu/drm/msm/dp/dp_display.c:402)
>>> dp_hpd_plug_handle.isra.0 (drivers/gpu/drm/msm/dp/dp_display.c:604)
>>> hpd_event_thread (drivers/gpu/drm/msm/dp/dp_display.c:1110)
>>> kthread (kernel/kthread.c:388)
>>> ret_from_fork (arch/arm64/kernel/entry.S:858)
>>
>> This only says where the oops happened, it doesn't necessarily in itself
>> indicate an oops at all or that in this case it's a NULL pointer
>> dereference.
>>
>> On the X13s I'm seeing the NULL deref in a different path during boot,
>> and when this happens after a deferred probe (due to the panel lookup
>> mess) it hangs the machine, which makes it a bit of a pain to debug:
>>
>> Unable to handle kernel NULL pointer dereference at virtual
>> address 0060
>> ...
>> CPU: 4 PID: 57 Comm: kworker/u16:1 Not tainted 6.7.0-rc1 #4
>> Hardware name: Qualcomm QRD, BIOS
>> 6.0.220110.BOOT.MXF.1.1-00470-MAKENA-1 01/10/2022
>> ...
>> Call trace:
>> drm_object_property_set_value+0x0/0x88 [drm]
>> dp_display_process_hpd_high+0xa0/0x14c [msm]
>> dp_hpd_plug_handle.constprop.0.isra.0+0x90/0x110 [msm]
>> dp_bridge_atomic_enable+0x184/0x21c [msm]
>> edp_bridge_atomic_enable+0x60/0x94 [msm]
>> drm_atomic_bridge_chain_enable+0x54/0xc8 [drm]
>> drm_atomic_helper_commit_modeset_enables+0x194/0x26c
>> [drm_kms_helper]
>> msm_atomic_commit_tail+0x204/0x804 [msm]
>> commit_tail+0xa4/0x18c [drm_kms_helper]
>> drm_atomic_helper_commit+0x19c/0x1b0 [drm_kms_helper]
>> drm_atomic_commit+0xa4/0x104 [drm]
>> drm_client_modeset_commit_atomic+0x22c/0x298 [drm]
>> drm_client_modeset_commit_locked+0x60/0x1c0 [drm]
>> drm_client_modeset_commit+0x30/0x58 [drm]
>> __drm_fb_helper_restore_fbdev_mode_unlocked+0xbc/0xfc
>> [drm_kms_helper]
>> drm_fb_helper_set_par+0x30/0x4c [drm_kms_helper]
>> fbcon_init+0x224/0x49c
>> visual_init+0xb0/0x108
>> do_bind_con_driver.isra.0+0x19c/0x38c
>> do_take_over_console+0x140/0x1ec
>> do_fbcon_takeover+0x6c/0xe4
>> fbcon_fb_registered+0x180/0x1f0
>> register_framebuffer+0x19c/0x228
>> __drm_fb_helper_initial_config_and_unlock+0x2e8/0x4e8
>> [drm_kms_helper]
>> drm_fb_helper_initial_config+0x3c/0x4c [drm_kms_helper]
>> msm_fbdev_client_hotplug+0x84/0xcc [msm]
>> drm_client_register+0x5c/0xa0 [drm]
>> msm_fbdev_setup+0x94/0x148 [msm]
>> msm_drm_bind+0x3d0/0x42c [msm]
>> try_to_bring_up_aggregate_device+0x1ec/0x2f4
>> __component_add+0xa8/0x194
>> component_add+0x14/0x20
>> dp_display_probe+0x278/0x41c [msm]
>>
>>> [1] https://patchwork.freedesktop.org/patch/30/
>>>
>>> Fixes: bfcc3d8f94f4 ("drm/msm/dp: support setting the DP subconnector
>>> type")
>>> Reviewed-by: Abhinav Kumar
>>> Signed-off-by: Dmitry Baryshkov
>>
>> Reviewed-by: Johan Hovold
>> Tested-by: Johan Hovold
>>
>
> Thanks !
>
>> Johan
[Freedreno] [pull] drm/msm: drm-msm-fixes-2023-11-21 for v6.7-rc3
Hi Dave, A few fixes for v6.7, description below The following changes since commit b08d26dac1a1075c874f40ee02ec8ddc39e20146: drm/msm/a7xx: actually use a7xx state registers (2023-10-16 09:38:56 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/msm.git tags/drm-msm-fixes-2023-11-21 for you to fetch changes up to 56466f653cb59a8f46e991ad1e285f43afdca7d4: drm/msm: remove unnecessary NULL check (2023-11-17 15:32:49 -0800) Fixes for v6.7-rc3: - Fix the VREG_CTRL_1 for 4nm CPHY to match downstream - Remove duplicate call to drm_kms_helper_poll_init() in msm_drm_init() - Fix the safe_lut_tbl[] for sc8280xp to match downstream - Don't attach the drm_dp_set_subconnector_property() for eDP - Fix to attach drm_dp_set_subconnector_property() for DP. Otherwise there is a bootup crash on multiple targets - Remove unnecessary NULL check left behind during cleanup Abel Vesa (1): drm/msm/dp: don't touch DP subconnector property in eDP case Bjorn Andersson (1): drm/msm/dpu: Add missing safe_lut_tbl in sc8280xp catalog Dan Carpenter (1): drm/msm: remove unnecessary NULL check Dmitry Baryshkov (2): drm/msm: remove exra drm_kms_helper_poll_init() call drm/msm/dp: attach the DP subconnector property Jonathan Marek (1): drm/msm/dsi: use the correct VREG_CTRL_1 value for 4nm cphy drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_8_0_sc8280xp.h | 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +-- drivers/gpu/drm/msm/dp/dp_display.c | 15 ++- drivers/gpu/drm/msm/dp/dp_drm.c | 3 +++ drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c| 2 +- drivers/gpu/drm/msm/msm_drv.c| 2 -- 6 files changed, 16 insertions(+), 10 deletions(-)
[Freedreno] [PATCH] drm/msm/a690: Fix reg values for a690
From: Danylo Piliaiev
KGSL doesn't support a690 so all reg values were the same as
on a660. Now we know the values and they are different from the
windows driver.
This fixes hangs on D3D12 games and some CTS tests.
Signed-off-by: Danylo Piliaiev
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 8176ea8da7a7..75e1ea0404d3 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1326,6 +1326,7 @@ static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
amsbc = 1;
rgb565_predicator = 1;
uavflagprd_inv = 2;
+ ubwc_mode = 2;
}
if (adreno_is_7c3(adreno_gpu)) {
@@ -1741,7 +1742,9 @@ static int hw_init(struct msm_gpu *gpu)
/* Setting the primFifo thresholds default values,
* and vccCacheSkipDis=1 bit (0x200) for A640 and newer
*/
- if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu) ||
adreno_is_a690(adreno_gpu))
+ if (adreno_is_a690(adreno_gpu))
+ gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00800200);
+ else if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu))
gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00300200);
else if (adreno_is_a640_family(adreno_gpu) || adreno_is_7c3(adreno_gpu))
gpu_write(gpu, REG_A6XX_PC_DBG_ECO_CNTL, 0x00200200);
@@ -1775,6 +1778,8 @@ static int hw_init(struct msm_gpu *gpu)
if (adreno_is_a730(adreno_gpu) ||
adreno_is_a740_family(adreno_gpu))
gpu_write(gpu, REG_A6XX_RBBM_INTERFACE_HANG_INT_CNTL, (1 << 30)
| 0xcf);
+ else if (adreno_is_a690(adreno_gpu))
+ gpu_write(gpu, REG_A6XX_RBBM_INTERFACE_HANG_INT_CNTL, (1 << 30)
| 0x4f);
else if (adreno_is_a619(adreno_gpu))
gpu_write(gpu, REG_A6XX_RBBM_INTERFACE_HANG_INT_CNTL, (1 << 30)
| 0x3f);
else if (adreno_is_a610(adreno_gpu))
@@ -1782,7 +1787,10 @@ static int hw_init(struct msm_gpu *gpu)
else
gpu_write(gpu, REG_A6XX_RBBM_INTERFACE_HANG_INT_CNTL, (1 << 30)
| 0x1f);
- gpu_write(gpu, REG_A6XX_UCHE_CLIENT_PF, 1);
+ if (adreno_is_a690(adreno_gpu))
+ gpu_write(gpu, REG_A6XX_UCHE_CLIENT_PF, 0x81);
+ else
+ gpu_write(gpu, REG_A6XX_UCHE_CLIENT_PF, 1);
/* Set weights for bicubic filtering */
if (adreno_is_a650_family(adreno_gpu)) {
@@ -1808,12 +1816,17 @@ static int hw_init(struct msm_gpu *gpu)
a6xx_set_cp_protect(gpu);
if (adreno_is_a660_family(adreno_gpu)) {
- gpu_write(gpu, REG_A6XX_CP_CHICKEN_DBG, 0x1);
+ if (adreno_is_a690(adreno_gpu))
+ gpu_write(gpu, REG_A6XX_CP_CHICKEN_DBG, 0x00028801);
+ else
+ gpu_write(gpu, REG_A6XX_CP_CHICKEN_DBG, 0x1);
gpu_write(gpu, REG_A6XX_RBBM_GBIF_CLIENT_QOS_CNTL, 0x0);
}
+ if (adreno_is_a690(adreno_gpu))
+ gpu_write(gpu, REG_A6XX_UCHE_CMDQ_CONFIG, 0x90);
/* Set dualQ + disable afull for A660 GPU */
- if (adreno_is_a660(adreno_gpu))
+ else if (adreno_is_a660(adreno_gpu))
gpu_write(gpu, REG_A6XX_UCHE_CMDQ_CONFIG, 0x66906);
else if (adreno_is_a7xx(adreno_gpu))
gpu_write(gpu, REG_A6XX_UCHE_CMDQ_CONFIG,
--
2.42.0
Re: [Freedreno] [Intel-gfx] [PATCH v4 00/20] remove I2C_CLASS_DDC support
On 21.11.2023 09:58, Jani Nikula wrote: > On Mon, 20 Nov 2023, Heiner Kallweit wrote: >> v4: >> - more ack and review tags > > Please do not send new versions just to record the acks and > reviews. They should be added while applying the patches. > Right, typically also patchwork interprets and shows A-b and R-b when sent as a reply to a patch of the series. I sent a new version because an A-b covered multiple patches and was sent as reply to the cover letter. > Thanks, > Jani. > Heiner
Re: [Freedreno] [PATCH v2] Remove custom dumb_map_offset implementation in msm driver
On Tue, 21 Nov 2023 at 04:26, Rob Clark wrote:
>
> On Wed, Nov 15, 2023 at 11:33 AM Dmitry Baryshkov
> wrote:
> >
> > On Wed, 15 Nov 2023 at 20:46, Dipam Turkar wrote:
> > >
> > > They are not outdated, my bad. I went through the locks' code and saw
> > > that they have been updated. But they are probably not necessary here as
> > > most of the drivers do not use any form of locking in their
> > > implementations. The generic implementations drm_gem_dumb_map_offset()
> > > and drm_gem_ttm_dumb_map_offset() do not have any locking mechanisms
> > > either.
> >
> > Excuse me, but this doesn't sound right to me. There are different
> > drivers with different implementations. So either we'd need a good
> > explanation of why it is not necessary, or this patch is NAKed.
>
> Digging a bit thru history, it looks like commit 0de23977cfeb
> ("drm/gem: convert to new unified vma manager") made external locking
> unnecessary, since the vma mgr already had it's own internal locking.
So, should we drop our own locking system?
>
> BR,
> -R
>
> > >
> > > Thanks and regards
> > > Dipam Turkar
> > >
> > > On Wed, Nov 15, 2023 at 8:37 PM Dmitry Baryshkov
> > > wrote:
> > >>
> > >> On Wed, 15 Nov 2023 at 16:30, Dipam Turkar wrote:
> > >> >
> > >> > Make msm use drm_gem_create_map_offset() instead of its custom
> > >> > implementation for associating GEM object with a fake offset. Since,
> > >> > we already have this generic implementation, we don't need the custom
> > >> > implementation and it is better to standardize the code for GEM based
> > >> > drivers. This also removes the outdated locking leftovers.
> > >>
> > >> Why are they outdated?
> > >>
> > >> >
> > >> > Signed-off-by: Dipam Turkar
> > >> > ---
> > >> > drivers/gpu/drm/msm/msm_drv.c | 2 +-
> > >> > drivers/gpu/drm/msm/msm_gem.c | 21 -
> > >> > drivers/gpu/drm/msm/msm_gem.h | 2 --
> > >> > 3 files changed, 1 insertion(+), 24 deletions(-)
> > >> >
> > >> > Changes in v2:
> > >> > Modify commit message to include the absence of internal locking
> > >> > leftovers
> > >> > around allocating a fake offset in msm_gem_mmap_offset() in the generic
> > >> > implementation drm_gem_create_map_offset().
> > >> >
> > >> > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> > >> > b/drivers/gpu/drm/msm/msm_drv.c
> > >> > index a428951ee539..86a15992c717 100644
> > >> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > >> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > >> > @@ -1085,7 +1085,7 @@ static const struct drm_driver msm_driver = {
> > >> > .open = msm_open,
> > >> > .postclose = msm_postclose,
> > >> > .dumb_create= msm_gem_dumb_create,
> > >> > - .dumb_map_offset= msm_gem_dumb_map_offset,
> > >> > + .dumb_map_offset= drm_gem_dumb_map_offset,
> > >> > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
> > >> > #ifdef CONFIG_DEBUG_FS
> > >> > .debugfs_init = msm_debugfs_init,
> > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.c
> > >> > b/drivers/gpu/drm/msm/msm_gem.c
> > >> > index db1e748daa75..489694ef79cb 100644
> > >> > --- a/drivers/gpu/drm/msm/msm_gem.c
> > >> > +++ b/drivers/gpu/drm/msm/msm_gem.c
> > >> > @@ -671,27 +671,6 @@ int msm_gem_dumb_create(struct drm_file *file,
> > >> > struct drm_device *dev,
> > >> > MSM_BO_SCANOUT | MSM_BO_WC, >handle,
> > >> > "dumb");
> > >> > }
> > >> >
> > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device
> > >> > *dev,
> > >> > - uint32_t handle, uint64_t *offset)
> > >> > -{
> > >> > - struct drm_gem_object *obj;
> > >> > - int ret = 0;
> > >> > -
> > >> > - /* GEM does all our handle to object mapping */
> > >> > - obj = drm_gem_object_lookup(file, handle);
> > >> > - if (obj == NULL) {
> > >> > - ret = -ENOENT;
> > >> > - goto fail;
> > >> > - }
> > >> > -
> > >> > - *offset = msm_gem_mmap_offset(obj);
> > >> > -
> > >> > - drm_gem_object_put(obj);
> > >> > -
> > >> > -fail:
> > >> > - return ret;
> > >> > -}
> > >> > -
> > >> > static void *get_vaddr(struct drm_gem_object *obj, unsigned madv)
> > >> > {
> > >> > struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.h
> > >> > b/drivers/gpu/drm/msm/msm_gem.h
> > >> > index 8ddef5443140..dc74a0ef865d 100644
> > >> > --- a/drivers/gpu/drm/msm/msm_gem.h
> > >> > +++ b/drivers/gpu/drm/msm/msm_gem.h
> > >> > @@ -139,8 +139,6 @@ struct page **msm_gem_pin_pages(struct
> > >> > drm_gem_object *obj);
> > >> > void msm_gem_unpin_pages(struct drm_gem_object *obj);
> > >> > int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
> > >> > struct drm_mode_create_dumb *args);
> > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device
> > >> > *dev,
> > >> > - uint32_t handle, uint64_t
Re: [Freedreno] [PATCH v6 6/6] usb: typec: qcom-pmic-typec: switch to DRM_AUX_HPD_BRIDGE
On Tue, 21 Nov 2023 at 12:46, Bryan O'Donoghue wrote: > > On 03/11/2023 23:03, Dmitry Baryshkov wrote: > > Use the freshly defined DRM_AUX_HPD_BRIDGE instead of open-coding the > > same functionality for the DRM bridge chain termination. > > > > Signed-off-by: Dmitry Baryshkov > > > + bridge_dev = drm_dp_hpd_bridge_register(tcpm->dev, > > to_of_node(tcpm->tcpc.fwnode)); > > + if (IS_ERR(bridge_dev)) > > + return PTR_ERR(bridge_dev); > > + > > What is the effect if we never attach any bridged devices ? > > We make an aux device that just hangs around and eventually get cleaned > up on release ? That's the way I read this code anyway. Yes. That's the point, to untangle the USB code and the DRM bridge. > Acked-by: Bryan O'Donoghue -- With best wishes Dmitry
Re: [Freedreno] [PATCH v6 6/6] usb: typec: qcom-pmic-typec: switch to DRM_AUX_HPD_BRIDGE
On 03/11/2023 23:03, Dmitry Baryshkov wrote: Use the freshly defined DRM_AUX_HPD_BRIDGE instead of open-coding the same functionality for the DRM bridge chain termination. Signed-off-by: Dmitry Baryshkov + bridge_dev = drm_dp_hpd_bridge_register(tcpm->dev, to_of_node(tcpm->tcpc.fwnode)); + if (IS_ERR(bridge_dev)) + return PTR_ERR(bridge_dev); + What is the effect if we never attach any bridged devices ? We make an aux device that just hangs around and eventually get cleaned up on release ? That's the way I read this code anyway. Acked-by: Bryan O'Donoghue
Re: [Freedreno] [Intel-gfx] [PATCH v4 00/20] remove I2C_CLASS_DDC support
On Mon, 20 Nov 2023, Heiner Kallweit wrote: > v4: > - more ack and review tags Please do not send new versions just to record the acks and reviews. They should be added while applying the patches. Thanks, Jani. -- Jani Nikula, Intel
Re: [Freedreno] [PATCH v6 1/6] drm/bridge: add transparent bridge helper
On 04/11/2023 00:03, Dmitry Baryshkov wrote:
Define a helper for creating simple transparent bridges which serve the
only purpose of linking devices into the bridge chain up to the last
bridge representing the connector. This is especially useful for
DP/USB-C bridge chains, which can span across several devices, but do
not require any additional functionality from the intermediate bridges.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/bridge/Kconfig | 9 ++
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/aux-bridge.c | 140
include/drm/bridge/aux-bridge.h | 19
4 files changed, 169 insertions(+)
create mode 100644 drivers/gpu/drm/bridge/aux-bridge.c
create mode 100644 include/drm/bridge/aux-bridge.h
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index ba82a1142adf..f12eab62799f 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -12,6 +12,15 @@ config DRM_PANEL_BRIDGE
help
DRM bridge wrapper of DRM panels
+config DRM_AUX_BRIDGE
+ tristate
+ depends on DRM_BRIDGE && OF
+ select AUXILIARY_BUS
+ select DRM_PANEL_BRIDGE
+ help
+ Simple transparent bridge that is used by several non-DRM drivers to
+ build bridges chain.
+
menu "Display Interface Bridges"
depends on DRM && DRM_BRIDGE
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 2b892b7ed59e..918e3bfff079 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,4 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_DRM_AUX_BRIDGE) += aux-bridge.o
obj-$(CONFIG_DRM_CHIPONE_ICN6211) += chipone-icn6211.o
obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o
obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o
diff --git a/drivers/gpu/drm/bridge/aux-bridge.c
b/drivers/gpu/drm/bridge/aux-bridge.c
new file mode 100644
index ..6245976b8fef
--- /dev/null
+++ b/drivers/gpu/drm/bridge/aux-bridge.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Author: Dmitry Baryshkov
+ */
+#include
+#include
+
+#include
+#include
+
+static DEFINE_IDA(drm_aux_bridge_ida);
+
+static void drm_aux_bridge_release(struct device *dev)
+{
+ struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+ ida_free(_aux_bridge_ida, adev->id);
+
+ kfree(adev);
+}
+
+static void drm_aux_bridge_unregister_adev(void *_adev)
+{
+ struct auxiliary_device *adev = _adev;
+
+ auxiliary_device_delete(adev);
+ auxiliary_device_uninit(adev);
+}
+
+/**
+ * drm_aux_bridge_register - Create a simple bridge device to link the chain
+ * @parent: device instance providing this bridge
+ *
+ * Creates a simple DRM bridge that doesn't implement any drm_bridge
+ * operations. Such bridges merely fill a place in the bridge chain linking
+ * surrounding DRM bridges.
+ *
+ * Return: zero on success, negative error code on failure
+ */
+int drm_aux_bridge_register(struct device *parent)
+{
+ struct auxiliary_device *adev;
+ int ret;
+
+ adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+ if (!adev)
+ return -ENOMEM;
+
+ ret = ida_alloc(_aux_bridge_ida, GFP_KERNEL);
+ if (ret < 0) {
+ kfree(adev);
+ return ret;
+ }
+
+ adev->id = ret;
+ adev->name = "aux_bridge";
+ adev->dev.parent = parent;
+ adev->dev.of_node = parent->of_node;
+ adev->dev.release = drm_aux_bridge_release;
+
+ ret = auxiliary_device_init(adev);
+ if (ret) {
+ ida_free(_aux_bridge_ida, adev->id);
+ kfree(adev);
+ return ret;
+ }
+
+ ret = auxiliary_device_add(adev);
+ if (ret) {
+ auxiliary_device_uninit(adev);
+ return ret;
+ }
+
+ return devm_add_action_or_reset(parent, drm_aux_bridge_unregister_adev,
adev);
+}
+EXPORT_SYMBOL_GPL(drm_aux_bridge_register);
+
+struct drm_aux_bridge_data {
+ struct drm_bridge bridge;
+ struct drm_bridge *next_bridge;
+ struct device *dev;
+};
+
+static int drm_aux_bridge_attach(struct drm_bridge *bridge,
+ enum drm_bridge_attach_flags flags)
+{
+ struct drm_aux_bridge_data *data;
+
+ if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
+ return -EINVAL;
+
+ data = container_of(bridge, struct drm_aux_bridge_data, bridge);
+
+ return drm_bridge_attach(bridge->encoder, data->next_bridge, bridge,
+DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+}
+
+static const struct drm_bridge_funcs drm_aux_bridge_funcs = {
+ .attach = drm_aux_bridge_attach,
+};
+
+static int drm_aux_bridge_probe(struct auxiliary_device *auxdev,
+ const struct auxiliary_device_id *id)
+{
+
Re: [Freedreno] [PATCH v6 4/6] drm/bridge: implement generic DP HPD bridge
On 04/11/2023 00:03, Dmitry Baryshkov wrote:
Several USB-C controllers implement a pretty simple DRM bridge which
implements just the HPD notification operations. Add special helper
for creating such simple bridges.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/bridge/Kconfig | 8 ++
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/aux-hpd-bridge.c | 164
include/drm/bridge/aux-bridge.h | 18 +++
4 files changed, 191 insertions(+)
create mode 100644 drivers/gpu/drm/bridge/aux-hpd-bridge.c
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index f12eab62799f..19d2dc05c397 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -21,6 +21,14 @@ config DRM_AUX_BRIDGE
Simple transparent bridge that is used by several non-DRM drivers to
build bridges chain.
+config DRM_AUX_HPD_BRIDGE
+ tristate
+ depends on DRM_BRIDGE && OF
+ select AUXILIARY_BUS
+ help
+ Simple bridge that terminates the bridge chain and provides HPD
+ support.
+
menu "Display Interface Bridges"
depends on DRM && DRM_BRIDGE
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 918e3bfff079..017b5832733b 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_DRM_AUX_BRIDGE) += aux-bridge.o
+obj-$(CONFIG_DRM_AUX_HPD_BRIDGE) += aux-hpd-bridge.o
obj-$(CONFIG_DRM_CHIPONE_ICN6211) += chipone-icn6211.o
obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o
obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o
diff --git a/drivers/gpu/drm/bridge/aux-hpd-bridge.c
b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
new file mode 100644
index ..4defac8ec63f
--- /dev/null
+++ b/drivers/gpu/drm/bridge/aux-hpd-bridge.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Author: Dmitry Baryshkov
+ */
+#include
+#include
+#include
+
+#include
+#include
+
+static DEFINE_IDA(drm_aux_hpd_bridge_ida);
+
+struct drm_aux_hpd_bridge_data {
+ struct drm_bridge bridge;
+ struct device *dev;
+};
+
+static void drm_aux_hpd_bridge_release(struct device *dev)
+{
+ struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+ ida_free(_aux_hpd_bridge_ida, adev->id);
+
+ of_node_put(adev->dev.platform_data);
+
+ kfree(adev);
+}
+
+static void drm_aux_hpd_bridge_unregister_adev(void *_adev)
+{
+ struct auxiliary_device *adev = _adev;
+
+ auxiliary_device_delete(adev);
+ auxiliary_device_uninit(adev);
+}
+
+/**
+ * drm_dp_hpd_bridge_register - Create a simple HPD DisplayPort bridge
+ * @parent: device instance providing this bridge
+ * @np: device node pointer corresponding to this bridge instance
+ *
+ * Creates a simple DRM bridge with the type set to
+ * DRM_MODE_CONNECTOR_DisplayPort, which terminates the bridge chain and is
+ * able to send the HPD events.
+ *
+ * Return: device instance that will handle created bridge or an error code
+ * encoded into the pointer.
+ */
+struct device *drm_dp_hpd_bridge_register(struct device *parent,
+ struct device_node *np)
+{
+ struct auxiliary_device *adev;
+ int ret;
+
+ adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+ if (!adev)
+ return ERR_PTR(-ENOMEM);
+
+ ret = ida_alloc(_aux_hpd_bridge_ida, GFP_KERNEL);
+ if (ret < 0) {
+ kfree(adev);
+ return ERR_PTR(ret);
+ }
+
+ adev->id = ret;
+ adev->name = "dp_hpd_bridge";
+ adev->dev.parent = parent;
+ adev->dev.of_node = parent->of_node;
+ adev->dev.release = drm_aux_hpd_bridge_release;
+ adev->dev.platform_data = np;
+
+ ret = auxiliary_device_init(adev);
+ if (ret) {
+ ida_free(_aux_hpd_bridge_ida, adev->id);
+ kfree(adev);
+ return ERR_PTR(ret);
+ }
+
+ ret = auxiliary_device_add(adev);
+ if (ret) {
+ auxiliary_device_uninit(adev);
+ return ERR_PTR(ret);
+ }
+
+ ret = devm_add_action_or_reset(parent,
drm_aux_hpd_bridge_unregister_adev, adev);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return >dev;
+
+}
+EXPORT_SYMBOL_GPL(drm_dp_hpd_bridge_register);
+
+/**
+ * drm_aux_hpd_bridge_notify - notify hot plug detection events
+ * @dev: device created for the HPD bridge
+ * @status: output connection status
+ *
+ * A wrapper around drm_bridge_hpd_notify() that is used to report hot plug
+ * detection events for bridges created via drm_dp_hpd_bridge_register().
+ *
+ * This function shall be called in a context that can sleep.
+ */
+void drm_aux_hpd_bridge_notify(struct device *dev, enum drm_connector_status
status)
+{
+ struct
Re: [Freedreno] [PATCH v6 6/6] usb: typec: qcom-pmic-typec: switch to DRM_AUX_HPD_BRIDGE
On 04/11/2023 01:03, Dmitry Baryshkov wrote:
Use the freshly defined DRM_AUX_HPD_BRIDGE instead of open-coding the
same functionality for the DRM bridge chain termination.
Signed-off-by: Dmitry Baryshkov
Greg, Bryan, could you pease ack merging this patch through the drm-misc
tree together with the rest of the series?
You have acked patch 3, but since that time I added this one.
---
drivers/usb/typec/tcpm/Kconfig| 1 +
drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c | 41 +++
2 files changed, 7 insertions(+), 35 deletions(-)
diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
index 0b2993fef564..64d5421c69e6 100644
--- a/drivers/usb/typec/tcpm/Kconfig
+++ b/drivers/usb/typec/tcpm/Kconfig
@@ -80,6 +80,7 @@ config TYPEC_QCOM_PMIC
tristate "Qualcomm PMIC USB Type-C Port Controller Manager driver"
depends on ARCH_QCOM || COMPILE_TEST
depends on DRM || DRM=n
+ select DRM_AUX_HPD_BRIDGE if DRM_BRIDGE
help
A Type-C port and Power Delivery driver which aggregates two
discrete pieces of silicon in the PM8150b PMIC block: the
diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
index 581199d37b49..1a2b4bddaa97 100644
--- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
+++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec.c
@@ -18,7 +18,7 @@
#include
#include
-#include
+#include
#include "qcom_pmic_typec_pdphy.h"
#include "qcom_pmic_typec_port.h"
@@ -36,7 +36,6 @@ struct pmic_typec {
struct pmic_typec_port *pmic_typec_port;
boolvbus_enabled;
struct mutexlock; /* VBUS state serialization */
- struct drm_bridge bridge;
};
#define tcpc_to_tcpm(_tcpc_) container_of(_tcpc_, struct pmic_typec, tcpc)
@@ -150,35 +149,6 @@ static int qcom_pmic_typec_init(struct tcpc_dev *tcpc)
return 0;
}
-#if IS_ENABLED(CONFIG_DRM)
-static int qcom_pmic_typec_attach(struct drm_bridge *bridge,
-enum drm_bridge_attach_flags flags)
-{
- return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
-}
-
-static const struct drm_bridge_funcs qcom_pmic_typec_bridge_funcs = {
- .attach = qcom_pmic_typec_attach,
-};
-
-static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm)
-{
- tcpm->bridge.funcs = _pmic_typec_bridge_funcs;
-#ifdef CONFIG_OF
- tcpm->bridge.of_node = of_get_child_by_name(tcpm->dev->of_node,
"connector");
-#endif
- tcpm->bridge.ops = DRM_BRIDGE_OP_HPD;
- tcpm->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
-
- return devm_drm_bridge_add(tcpm->dev, >bridge);
-}
-#else
-static int qcom_pmic_typec_init_drm(struct pmic_typec *tcpm)
-{
- return 0;
-}
-#endif
-
static int qcom_pmic_typec_probe(struct platform_device *pdev)
{
struct pmic_typec *tcpm;
@@ -186,6 +156,7 @@ static int qcom_pmic_typec_probe(struct platform_device
*pdev)
struct device_node *np = dev->of_node;
const struct pmic_typec_resources *res;
struct regmap *regmap;
+ struct device *bridge_dev;
u32 base[2];
int ret;
@@ -241,14 +212,14 @@ static int qcom_pmic_typec_probe(struct platform_device *pdev)
mutex_init(>lock);
platform_set_drvdata(pdev, tcpm);
- ret = qcom_pmic_typec_init_drm(tcpm);
- if (ret)
- return ret;
-
tcpm->tcpc.fwnode = device_get_named_child_node(tcpm->dev, "connector");
if (!tcpm->tcpc.fwnode)
return -EINVAL;
+ bridge_dev = drm_dp_hpd_bridge_register(tcpm->dev, to_of_node(tcpm->tcpc.fwnode));
+ if (IS_ERR(bridge_dev))
+ return PTR_ERR(bridge_dev);
+
tcpm->tcpm_port = tcpm_register_port(tcpm->dev, >tcpc);
if (IS_ERR(tcpm->tcpm_port)) {
ret = PTR_ERR(tcpm->tcpm_port);
--
With best wishes
Dmitry
