Re: [PATCH] WIP: drm/amd/display: dc/irq: use dce_6_0_{d,sh_mask}.h
Hi,
just not to keep you engaged on this,
I've got the vblank irq handling working now.
I had exactly to replicate in dce60_irq_service.c the behavior of commit
b10d51f8 ("drm/amd/display: Add interrupt entries for VBLANK isr.")
and behavior of b57de80a ("drm/amd/display: Register on VLBLANK ISR.")
in amdgpu_dm.c
From functional point of view IRQ handling for DCE6 is complete,
I can proceed with other improvements, like Line Buffer/Watermark programming,
and VGA support for Kaveri and older.
Some suggestion those two areas by AMD developers would be very much
appreciated.
Kind regards
Mauro
On Wed, Jan 9, 2019 at 8:07 PM Mauro Rossi wrote:
>
> Ah-ha! (Meaning self-checking and trying to self-correct myself)
>
> I've seen that commit b10d51f8 ("drm/amd/display: Add interrupt
> entries for VBLANK isr.")
> required to be complemented by b57de80a ("drm/amd/display: Register on
> VLBLANK ISR.")
> which changed vblank irq control in dce110_register_irq_handlers() in file
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>
> In this WIP I did not applied changes in amdgpu_dm_irq.c accordingly
> and this is the most probable root cause for screen being always blank
>
> I will specialize a dce60_register_irq_handlers()
> within braces #if defined(CONFIG_DRM_AMD_DC_SI)/#endif
> and replicate the behavior of b57de80a ("drm/amd/display: Register on
> VLBLANK ISR.")
>
> Please let me know if I'm at least going in the right direction
> and confirm that there is no other VBLANK Interrupt Register to be used in
> DCE6,
> if there is some other VBLANK Interrupt Register better than the one I found
> I'd like to know to implement directly the best solution.
>
> Mauro
>
> On Wed, Jan 2, 2019 at 1:12 AM Mauro Rossi wrote:
> >
> > Work In Progress for using DCE6 headers
> > vblank registers and masks where identified,
> > but using them gives a glipse with monitor screen active,
> > followed by monitor screen in standby
> >
> > Please review to identify the problem
> > as the DCE6 vblank irq do not map exactlyto DC irq code,
> > it's not clear how to_dal_irq_source_dce60 should be defined and used
> > and it's not clear how to manage the DCE6 registers
> > in the following struct:
> >
> > static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> > .set = dce110_vblank_set,
> > .ack = NULL
> > };
> > ---
> > .../display/dc/irq/dce60/irq_service_dce60.c | 116 +++---
> > .../display/dc/irq/dce60/irq_service_dce60.h | 5 +
> > .../drm/amd/include/asic_reg/dce/dce_6_0_d.h | 14 +++
> > .../include/asic_reg/dce/dce_6_0_sh_mask.h| 24
> > 4 files changed, 145 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
> > b/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
> > index 107e0dcb5f80..c3befab49374 100644
> > --- a/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
> > +++ b/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
> > @@ -30,10 +30,17 @@
> > #include "irq_service_dce60.h"
> > #include "../dce110/irq_service_dce110.h"
> >
> > -#include "dce/dce_8_0_d.h"
> > -#include "dce/dce_8_0_sh_mask.h"
> > -
> > +#include "dce/dce_6_0_d.h"
> > +#include "dce/dce_6_0_sh_mask.h"
> > +/* Q1: Are the DCE8 Interrupt Vector tables applicable to DCE6? */
> > #include "ivsrcid/ivsrcid_vislands30.h"
> > +/* See b10d51f8 ("drm/amd/display: Add interrupt entries for VBLANK isr.")
> > */
> > +#define VISLANDS30_IV_SRCID_D1_VBLANK1
> > +#define VISLANDS30_IV_SRCID_D2_VBLANK2
> > +#define VISLANDS30_IV_SRCID_D3_VBLANK3
> > +#define VISLANDS30_IV_SRCID_D4_VBLANK4
> > +#define VISLANDS30_IV_SRCID_D5_VBLANK5
> > +#define VISLANDS30_IV_SRCID_D6_VBLANK6
> >
> > #include "dc_types.h"
> >
> > @@ -78,7 +85,7 @@ static const struct irq_source_info_funcs
> > pflip_irq_info_funcs = {
> > .set = NULL,
> > .ack = NULL
> > };
> > -
> > + /* NOTE: .set = NULL in commit b10d51f8 Q2: Can dce110_vblank_set be used
> > here? */
> > static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> > .set = dce110_vblank_set,
> > .ack = NULL
> > @@ -145,21 +152,21 @@ static const struct irq_source_info_funcs
> > vblank_irq_info_funcs = {
> > .funcs = _irq_info_funcs\
> > }
> >
> > +/* NOTE: vblank_irq_info_funcs.set = dce110_vblank instead of NULL (see
> > b10d51f8) */
> > #define vblank_int_entry(reg_num)\
> > [DC_IRQ_SOURCE_VBLANK1 + reg_num] = {\
> > - .enable_reg = mmCRTC ## reg_num ##
> > _CRTC_VERTICAL_INTERRUPT0_CONTROL,\
> > + .enable_reg = mmLB ## reg_num ## _INT_MASK,\
> > .enable_mask =\
> > -
> > CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_INT_ENABLE_MASK,\
> > +
Re: [PATCH] WIP: drm/amd/display: dc/irq: use dce_6_0_{d,sh_mask}.h
Ah-ha! (Meaning self-checking and trying to self-correct myself)
I've seen that commit b10d51f8 ("drm/amd/display: Add interrupt
entries for VBLANK isr.")
required to be complemented by b57de80a ("drm/amd/display: Register on
VLBLANK ISR.")
which changed vblank irq control in dce110_register_irq_handlers() in file
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
In this WIP I did not applied changes in amdgpu_dm_irq.c accordingly
and this is the most probable root cause for screen being always blank
I will specialize a dce60_register_irq_handlers()
within braces #if defined(CONFIG_DRM_AMD_DC_SI)/#endif
and replicate the behavior of b57de80a ("drm/amd/display: Register on
VLBLANK ISR.")
Please let me know if I'm at least going in the right direction
and confirm that there is no other VBLANK Interrupt Register to be used in DCE6,
if there is some other VBLANK Interrupt Register better than the one I found
I'd like to know to implement directly the best solution.
Mauro
On Wed, Jan 2, 2019 at 1:12 AM Mauro Rossi wrote:
>
> Work In Progress for using DCE6 headers
> vblank registers and masks where identified,
> but using them gives a glipse with monitor screen active,
> followed by monitor screen in standby
>
> Please review to identify the problem
> as the DCE6 vblank irq do not map exactlyto DC irq code,
> it's not clear how to_dal_irq_source_dce60 should be defined and used
> and it's not clear how to manage the DCE6 registers
> in the following struct:
>
> static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> .set = dce110_vblank_set,
> .ack = NULL
> };
> ---
> .../display/dc/irq/dce60/irq_service_dce60.c | 116 +++---
> .../display/dc/irq/dce60/irq_service_dce60.h | 5 +
> .../drm/amd/include/asic_reg/dce/dce_6_0_d.h | 14 +++
> .../include/asic_reg/dce/dce_6_0_sh_mask.h| 24
> 4 files changed, 145 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
> b/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
> index 107e0dcb5f80..c3befab49374 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
> @@ -30,10 +30,17 @@
> #include "irq_service_dce60.h"
> #include "../dce110/irq_service_dce110.h"
>
> -#include "dce/dce_8_0_d.h"
> -#include "dce/dce_8_0_sh_mask.h"
> -
> +#include "dce/dce_6_0_d.h"
> +#include "dce/dce_6_0_sh_mask.h"
> +/* Q1: Are the DCE8 Interrupt Vector tables applicable to DCE6? */
> #include "ivsrcid/ivsrcid_vislands30.h"
> +/* See b10d51f8 ("drm/amd/display: Add interrupt entries for VBLANK isr.") */
> +#define VISLANDS30_IV_SRCID_D1_VBLANK1
> +#define VISLANDS30_IV_SRCID_D2_VBLANK2
> +#define VISLANDS30_IV_SRCID_D3_VBLANK3
> +#define VISLANDS30_IV_SRCID_D4_VBLANK4
> +#define VISLANDS30_IV_SRCID_D5_VBLANK5
> +#define VISLANDS30_IV_SRCID_D6_VBLANK6
>
> #include "dc_types.h"
>
> @@ -78,7 +85,7 @@ static const struct irq_source_info_funcs
> pflip_irq_info_funcs = {
> .set = NULL,
> .ack = NULL
> };
> -
> + /* NOTE: .set = NULL in commit b10d51f8 Q2: Can dce110_vblank_set be used
> here? */
> static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> .set = dce110_vblank_set,
> .ack = NULL
> @@ -145,21 +152,21 @@ static const struct irq_source_info_funcs
> vblank_irq_info_funcs = {
> .funcs = _irq_info_funcs\
> }
>
> +/* NOTE: vblank_irq_info_funcs.set = dce110_vblank instead of NULL (see
> b10d51f8) */
> #define vblank_int_entry(reg_num)\
> [DC_IRQ_SOURCE_VBLANK1 + reg_num] = {\
> - .enable_reg = mmCRTC ## reg_num ##
> _CRTC_VERTICAL_INTERRUPT0_CONTROL,\
> + .enable_reg = mmLB ## reg_num ## _INT_MASK,\
> .enable_mask =\
> -
> CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_INT_ENABLE_MASK,\
> + INT_MASK__VBLANK_INT_MASK,\
> .enable_value = {\
> -
> CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_INT_ENABLE_MASK,\
> -
> ~CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_INT_ENABLE_MASK},\
> - .ack_reg = mmCRTC ## reg_num ##
> _CRTC_VERTICAL_INTERRUPT0_CONTROL,\
> + INT_MASK__VBLANK_INT_MASK,\
> + ~INT_MASK__VBLANK_INT_MASK},\
> + .ack_reg = mmLB ## reg_num ## _VBLANK_STATUS,\
> .ack_mask =\
> -
> CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_CLEAR_MASK,\
> + VBLANK_STATUS__VBLANK_ACK_MASK,\
> .ack_value =\
> -
> CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_CLEAR_MASK,\
> - .funcs =
[PATCH] WIP: drm/amd/display: dc/irq: use dce_6_0_{d,sh_mask}.h
Work In Progress for using DCE6 headers
vblank registers and masks where identified,
but using them gives a glipse with monitor screen active,
followed by monitor screen in standby
Please review to identify the problem
as the DCE6 vblank irq do not map exactlyto DC irq code,
it's not clear how to_dal_irq_source_dce60 should be defined and used
and it's not clear how to manage the DCE6 registers
in the following struct:
static const struct irq_source_info_funcs vblank_irq_info_funcs = {
.set = dce110_vblank_set,
.ack = NULL
};
---
.../display/dc/irq/dce60/irq_service_dce60.c | 116 +++---
.../display/dc/irq/dce60/irq_service_dce60.h | 5 +
.../drm/amd/include/asic_reg/dce/dce_6_0_d.h | 14 +++
.../include/asic_reg/dce/dce_6_0_sh_mask.h| 24
4 files changed, 145 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
b/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
index 107e0dcb5f80..c3befab49374 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dce60/irq_service_dce60.c
@@ -30,10 +30,17 @@
#include "irq_service_dce60.h"
#include "../dce110/irq_service_dce110.h"
-#include "dce/dce_8_0_d.h"
-#include "dce/dce_8_0_sh_mask.h"
-
+#include "dce/dce_6_0_d.h"
+#include "dce/dce_6_0_sh_mask.h"
+/* Q1: Are the DCE8 Interrupt Vector tables applicable to DCE6? */
#include "ivsrcid/ivsrcid_vislands30.h"
+/* See b10d51f8 ("drm/amd/display: Add interrupt entries for VBLANK isr.") */
+#define VISLANDS30_IV_SRCID_D1_VBLANK1
+#define VISLANDS30_IV_SRCID_D2_VBLANK2
+#define VISLANDS30_IV_SRCID_D3_VBLANK3
+#define VISLANDS30_IV_SRCID_D4_VBLANK4
+#define VISLANDS30_IV_SRCID_D5_VBLANK5
+#define VISLANDS30_IV_SRCID_D6_VBLANK6
#include "dc_types.h"
@@ -78,7 +85,7 @@ static const struct irq_source_info_funcs
pflip_irq_info_funcs = {
.set = NULL,
.ack = NULL
};
-
+ /* NOTE: .set = NULL in commit b10d51f8 Q2: Can dce110_vblank_set be used
here? */
static const struct irq_source_info_funcs vblank_irq_info_funcs = {
.set = dce110_vblank_set,
.ack = NULL
@@ -145,21 +152,21 @@ static const struct irq_source_info_funcs
vblank_irq_info_funcs = {
.funcs = _irq_info_funcs\
}
+/* NOTE: vblank_irq_info_funcs.set = dce110_vblank instead of NULL (see
b10d51f8) */
#define vblank_int_entry(reg_num)\
[DC_IRQ_SOURCE_VBLANK1 + reg_num] = {\
- .enable_reg = mmCRTC ## reg_num ##
_CRTC_VERTICAL_INTERRUPT0_CONTROL,\
+ .enable_reg = mmLB ## reg_num ## _INT_MASK,\
.enable_mask =\
-
CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_INT_ENABLE_MASK,\
+ INT_MASK__VBLANK_INT_MASK,\
.enable_value = {\
-
CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_INT_ENABLE_MASK,\
-
~CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_INT_ENABLE_MASK},\
- .ack_reg = mmCRTC ## reg_num ##
_CRTC_VERTICAL_INTERRUPT0_CONTROL,\
+ INT_MASK__VBLANK_INT_MASK,\
+ ~INT_MASK__VBLANK_INT_MASK},\
+ .ack_reg = mmLB ## reg_num ## _VBLANK_STATUS,\
.ack_mask =\
-
CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_CLEAR_MASK,\
+ VBLANK_STATUS__VBLANK_ACK_MASK,\
.ack_value =\
-
CRTC_VERTICAL_INTERRUPT0_CONTROL__CRTC_VERTICAL_INTERRUPT0_CLEAR_MASK,\
- .funcs = _irq_info_funcs,\
- .src_id = VISLANDS30_IV_SRCID_D1_VERTICAL_INTERRUPT0 + reg_num\
+ VBLANK_STATUS__VBLANK_ACK_MASK,\
+ .funcs = _irq_info_funcs\
}
#define dummy_irq_entry() \
@@ -273,8 +280,89 @@ irq_source_info_dce60[DAL_IRQ_SOURCES_NUMBER] = {
vblank_int_entry(5),
};
+/* to_dal_irq_source_dce60 treats VBLANK differently from common dce110 one */
+enum dc_irq_source to_dal_irq_source_dce60(
+ struct irq_service *irq_service,
+ uint32_t src_id,
+ uint32_t ext_id)
+{
+ switch (src_id) {
+ case VISLANDS30_IV_SRCID_D1_VBLANK:
+ return DC_IRQ_SOURCE_VBLANK1;
+ case VISLANDS30_IV_SRCID_D2_VBLANK:
+ return DC_IRQ_SOURCE_VBLANK2;
+ case VISLANDS30_IV_SRCID_D3_VBLANK:
+ return DC_IRQ_SOURCE_VBLANK3;
+ case VISLANDS30_IV_SRCID_D4_VBLANK:
+ return DC_IRQ_SOURCE_VBLANK4;
+ case VISLANDS30_IV_SRCID_D5_VBLANK:
+ return DC_IRQ_SOURCE_VBLANK5;
+ case VISLANDS30_IV_SRCID_D6_VBLANK:
+ return DC_IRQ_SOURCE_VBLANK6;
+ case VISLANDS30_IV_SRCID_D1_V_UPDATE_INT:
+ return
