[PATCH v4 0/7] drm/tidss: Use new connector model for tidss

[PATCH v4 0/7] drm/tidss: Use new connector model for tidss

[Nikhil Devshatwar]

This series moves the tidss to using new connectoe model, where the 
SoC driver (tidss) creates the connector and all the bridges are 
attached with the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR

Since the bridges do not create the connector, the bus_format and
bus_flag is set via atomic hooks.
Support format negotiations in the tfp410 and mhdp bridge drivers
as a first step before moving the connector model.

Nikhil Devshatwar (7):
  drm/bridge: tfp410: Support format negotiation hooks
  drm/bridge: tfp410: Set input_bus_flags in atomic_check
  drm/bridge: mhdp8546: Add minimal format negotiation
  drm/bridge: mhdp8546: Set input_bus_flags from atomic_check
  drm/tidss: Set bus_format correctly from bridge/connector
  drm/tidss: Move to newer connector model
  drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable

 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 75 +++++++++++++------
 drivers/gpu/drm/bridge/ti-tfp410.c            | 49 ++++++++++++
 drivers/gpu/drm/tidss/tidss_drv.h             |  3 +
 drivers/gpu/drm/tidss/tidss_encoder.c         | 36 ++++-----
 drivers/gpu/drm/tidss/tidss_kms.c             | 19 ++++-
 5 files changed, 137 insertions(+), 45 deletions(-)

-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 1/7] drm/bridge: tfp410: Support format negotiation hooks

[Nikhil Devshatwar]

With new connector model, tfp410 will not create the connector and
SoC driver will rely on format negotiation to setup the encoder format.

Support format negotiations hooks in the drm_bridge_funcs.
Use helper functions for state management.

Output format is expected to be MEDIA_BUS_FMT_FIXED
Input format is the one selected by the bridge from DT properties.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

Notes:
    changes from v1:
    * Use only MEDIA_BUS_FMT_FIXED for output

 drivers/gpu/drm/bridge/ti-tfp410.c | 33 ++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index ba3fa2a9b8a4..3def9acba86b 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -204,12 +204,45 @@ static enum drm_mode_status tfp410_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
+static u32 *tfp410_get_input_bus_fmts(struct drm_bridge *bridge,
+				      struct drm_bridge_state *bridge_state,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_connector_state *conn_state,
+				      u32 output_fmt,
+				      unsigned int *num_input_fmts)
+{
+	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
+	u32 *input_fmts;
+
+	*num_input_fmts = 0;
+
+	/*
+	 * Output of tfp410 is DVI, which is TMDS.
+	 * There is no media format defined for this.
+	 * Using MEDIA_BUS_FMT_FIXED for now.
+	 */
+	if (output_fmt != MEDIA_BUS_FMT_FIXED)
+		return NULL;
+
+	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	*num_input_fmts = 1;
+	input_fmts[0] = dvi->bus_format;
+	return input_fmts;
+}
+
 static const struct drm_bridge_funcs tfp410_bridge_funcs = {
 	.attach		= tfp410_attach,
 	.detach		= tfp410_detach,
 	.enable		= tfp410_enable,
 	.disable	= tfp410_disable,
 	.mode_valid	= tfp410_mode_valid,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_get_input_bus_fmts = tfp410_get_input_bus_fmts,
 };
 
 static const struct drm_bridge_timings tfp410_default_timings = {
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 2/7] drm/bridge: tfp410: Set input_bus_flags in atomic_check

[Nikhil Devshatwar]

input_bus_flags are specified in drm_bridge_timings (legacy) as well
as drm_bridge_state->input_bus_cfg.flags

The flags from the timings will be deprecated. Bridges are supposed
to validate and set the bridge state flags from atomic_check.

Implement atomic_check hook for the same.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index 3def9acba86b..4c536df003c8 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -233,6 +233,20 @@ static u32 *tfp410_get_input_bus_fmts(struct drm_bridge *bridge,
 	return input_fmts;
 }
 
+int tfp410_atomic_check(struct drm_bridge *bridge,
+		    struct drm_bridge_state *bridge_state,
+		    struct drm_crtc_state *crtc_state,
+		    struct drm_connector_state *conn_state)
+{
+	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
+
+	/*
+	 * There might be flags negotiation supported in future
+	 * Set the bus flags in atomic_check statically for now
+	 */
+	bridge_state->input_bus_cfg.flags = dvi->timings.input_bus_flags;
+}
+
 static const struct drm_bridge_funcs tfp410_bridge_funcs = {
 	.attach		= tfp410_attach,
 	.detach		= tfp410_detach,
@@ -243,6 +257,7 @@ static const struct drm_bridge_funcs tfp410_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_get_input_bus_fmts = tfp410_get_input_bus_fmts,
+	.atomic_check = tfp410_atomic_check,
 };
 
 static const struct drm_bridge_timings tfp410_default_timings = {
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/7] drm/bridge: tfp410: Set input_bus_flags in atomic_check

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]

Hi Nikhil,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.10-rc6 next-20201201]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nikhil-Devshatwar/drm-tidss-Use-new-connector-model-for-tidss/20201201-202125
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: mips-randconfig-r023-20201201 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ac40a2d8f16b8a8c68fc811d67f647740e965cb8)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/936e9f4e0dd9ad362b11e4cb6a240cdaec2b8b28
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nikhil-Devshatwar/drm-tidss-Use-new-connector-model-for-tidss/20201201-202125
        git checkout 936e9f4e0dd9ad362b11e4cb6a240cdaec2b8b28
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/ti-tfp410.c:236:5: warning: no previous prototype for function 'tfp410_atomic_check' [-Wmissing-prototypes]
   int tfp410_atomic_check(struct drm_bridge *bridge,
       ^
   drivers/gpu/drm/bridge/ti-tfp410.c:236:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int tfp410_atomic_check(struct drm_bridge *bridge,
   ^
   static 
   drivers/gpu/drm/bridge/ti-tfp410.c:248:1: error: non-void function does not return a value [-Werror,-Wreturn-type]
   }
   ^
   1 warning and 1 error generated.

vim +/tfp410_atomic_check +236 drivers/gpu/drm/bridge/ti-tfp410.c

   235	
 > 236	int tfp410_atomic_check(struct drm_bridge *bridge,
   237			    struct drm_bridge_state *bridge_state,
   238			    struct drm_crtc_state *crtc_state,
   239			    struct drm_connector_state *conn_state)
   240	{
   241		struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
   242	
   243		/*
   244		 * There might be flags negotiation supported in future
   245		 * Set the bus flags in atomic_check statically for now
   246		 */
   247		bridge_state->input_bus_cfg.flags = dvi->timings.input_bus_flags;
   248	}
   249	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24386 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/7] drm/bridge: tfp410: Set input_bus_flags in atomic_check

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3110 bytes --]

Hi Nikhil,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.10-rc6 next-20201201]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nikhil-Devshatwar/drm-tidss-Use-new-connector-model-for-tidss/20201201-202125
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: mips-randconfig-r023-20201201 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ac40a2d8f16b8a8c68fc811d67f647740e965cb8)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/936e9f4e0dd9ad362b11e4cb6a240cdaec2b8b28
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nikhil-Devshatwar/drm-tidss-Use-new-connector-model-for-tidss/20201201-202125
        git checkout 936e9f4e0dd9ad362b11e4cb6a240cdaec2b8b28
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/ti-tfp410.c:236:5: warning: no previous prototype for function 'tfp410_atomic_check' [-Wmissing-prototypes]
   int tfp410_atomic_check(struct drm_bridge *bridge,
       ^
   drivers/gpu/drm/bridge/ti-tfp410.c:236:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int tfp410_atomic_check(struct drm_bridge *bridge,
   ^
   static 
   drivers/gpu/drm/bridge/ti-tfp410.c:248:1: error: non-void function does not return a value [-Werror,-Wreturn-type]
   }
   ^
   1 warning and 1 error generated.

vim +/tfp410_atomic_check +236 drivers/gpu/drm/bridge/ti-tfp410.c

   235	
 > 236	int tfp410_atomic_check(struct drm_bridge *bridge,
   237			    struct drm_bridge_state *bridge_state,
   238			    struct drm_crtc_state *crtc_state,
   239			    struct drm_connector_state *conn_state)
   240	{
   241		struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
   242	
   243		/*
   244		 * There might be flags negotiation supported in future
   245		 * Set the bus flags in atomic_check statically for now
   246		 */
   247		bridge_state->input_bus_cfg.flags = dvi->timings.input_bus_flags;
   248	}
   249	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 24386 bytes --]

[PATCH v5] drm/bridge: tfp410: Set input_bus_flags in atomic_check

[Nikhil Devshatwar]

input_bus_flags are specified in drm_bridge_timings (legacy) as well
as drm_bridge_state->input_bus_cfg.flags

The flags from the timings will be deprecated. Bridges are supposed
to validate and set the bridge state flags from atomic_check.

Implement atomic_check hook for the same.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---

Notes:
    changes from v4:
    * fix a warning Reported-by: kernel test robot <lkp@intel.com>

 drivers/gpu/drm/bridge/ti-tfp410.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index 3def9acba86b..92963d12106b 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -233,6 +233,20 @@ static u32 *tfp410_get_input_bus_fmts(struct drm_bridge *bridge,
 	return input_fmts;
 }
 
+static int tfp410_atomic_check(struct drm_bridge *bridge,
+		    struct drm_bridge_state *bridge_state,
+		    struct drm_crtc_state *crtc_state,
+		    struct drm_connector_state *conn_state)
+{
+	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
+
+	/*
+	 * There might be flags negotiation supported in future
+	 * Set the bus flags in atomic_check statically for now
+	 */
+	bridge_state->input_bus_cfg.flags = dvi->timings.input_bus_flags;
+}
+
 static const struct drm_bridge_funcs tfp410_bridge_funcs = {
 	.attach		= tfp410_attach,
 	.detach		= tfp410_detach,
@@ -243,6 +257,7 @@ static const struct drm_bridge_funcs tfp410_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_get_input_bus_fmts = tfp410_get_input_bus_fmts,
+	.atomic_check = tfp410_atomic_check,
 };
 
 static const struct drm_bridge_timings tfp410_default_timings = {
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5] drm/bridge: tfp410: Set input_bus_flags in atomic_check

[Boris Brezillon]

On Thu, 3 Dec 2020 18:20:48 +0530
Nikhil Devshatwar <nikhil.nd@ti.com> wrote:

> input_bus_flags are specified in drm_bridge_timings (legacy) as well
> as drm_bridge_state->input_bus_cfg.flags
> 
> The flags from the timings will be deprecated. Bridges are supposed
> to validate and set the bridge state flags from atomic_check.
> 
> Implement atomic_check hook for the same.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
> 
> Notes:
>     changes from v4:
>     * fix a warning Reported-by: kernel test robot <lkp@intel.com>
> 
>  drivers/gpu/drm/bridge/ti-tfp410.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index 3def9acba86b..92963d12106b 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -233,6 +233,20 @@ static u32 *tfp410_get_input_bus_fmts(struct drm_bridge *bridge,
>  	return input_fmts;
>  }
>  
> +static int tfp410_atomic_check(struct drm_bridge *bridge,
> +		    struct drm_bridge_state *bridge_state,
> +		    struct drm_crtc_state *crtc_state,
> +		    struct drm_connector_state *conn_state)
> +{
> +	struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
> +
> +	/*
> +	 * There might be flags negotiation supported in future
> +	 * Set the bus flags in atomic_check statically for now
> +	 */
> +	bridge_state->input_bus_cfg.flags = dvi->timings.input_bus_flags;

The return statement is still missing :-).

> +}
> +
>  static const struct drm_bridge_funcs tfp410_bridge_funcs = {
>  	.attach		= tfp410_attach,
>  	.detach		= tfp410_detach,
> @@ -243,6 +257,7 @@ static const struct drm_bridge_funcs tfp410_bridge_funcs = {
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>  	.atomic_get_input_bus_fmts = tfp410_get_input_bus_fmts,
> +	.atomic_check = tfp410_atomic_check,
>  };
>  
>  static const struct drm_bridge_timings tfp410_default_timings = {

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 3/7] drm/bridge: mhdp8546: Add minimal format negotiation

[Nikhil Devshatwar]

With new connector model, mhdp bridge will not create the connector and
SoC driver will rely on format negotiation to setup the encoder format.

Support minimal format negotiations hooks in the drm_bridge_funcs.
Complete format negotiation can be added based on EDID data.
This patch adds the minimal required support to avoid failure
after moving to new connector model.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---

Notes:
    changes from v1:
    * cosmetic fixes, commit message update

 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index d0c65610ebb5..2cd809eed827 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2078,6 +2078,30 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge *bridge)
 	return &cdns_mhdp_state->base;
 }
 
+static u32 *cdns_mhdp_get_input_bus_fmts(struct drm_bridge *bridge,
+				      struct drm_bridge_state *bridge_state,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_connector_state *conn_state,
+				      u32 output_fmt,
+				      unsigned int *num_input_fmts)
+{
+	u32 *input_fmts;
+	u32 default_bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
+
+	*num_input_fmts = 0;
+
+	if (output_fmt != MEDIA_BUS_FMT_FIXED)
+		return NULL;
+
+	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	*num_input_fmts = 1;
+	input_fmts[0] = default_bus_format;
+	return input_fmts;
+}
+
 static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
 				  struct drm_bridge_state *bridge_state,
 				  struct drm_crtc_state *crtc_state,
@@ -2142,6 +2166,7 @@ static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
 	.atomic_duplicate_state = cdns_mhdp_bridge_atomic_duplicate_state,
 	.atomic_destroy_state = cdns_mhdp_bridge_atomic_destroy_state,
 	.atomic_reset = cdns_mhdp_bridge_atomic_reset,
+	.atomic_get_input_bus_fmts = cdns_mhdp_get_input_bus_fmts,
 	.detect = cdns_mhdp_bridge_detect,
 	.get_edid = cdns_mhdp_bridge_get_edid,
 	.hpd_enable = cdns_mhdp_bridge_hpd_enable,
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 3/7] drm/bridge: mhdp8546: Add minimal format negotiation

[Boris Brezillon]

On Tue, 1 Dec 2020 17:48:26 +0530
Nikhil Devshatwar <nikhil.nd@ti.com> wrote:

> With new connector model, mhdp bridge will not create the connector and
> SoC driver will rely on format negotiation to setup the encoder format.
> 
> Support minimal format negotiations hooks in the drm_bridge_funcs.
> Complete format negotiation can be added based on EDID data.
> This patch adds the minimal required support to avoid failure
> after moving to new connector model.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> 
> Notes:
>     changes from v1:
>     * cosmetic fixes, commit message update
> 
>  .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index d0c65610ebb5..2cd809eed827 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2078,6 +2078,30 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge *bridge)
>  	return &cdns_mhdp_state->base;
>  }
>  
> +static u32 *cdns_mhdp_get_input_bus_fmts(struct drm_bridge *bridge,
> +				      struct drm_bridge_state *bridge_state,
> +				      struct drm_crtc_state *crtc_state,
> +				      struct drm_connector_state *conn_state,
> +				      u32 output_fmt,
> +				      unsigned int *num_input_fmts)
> +{
> +	u32 *input_fmts;
> +	u32 default_bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
> +
> +	*num_input_fmts = 0;
> +
> +	if (output_fmt != MEDIA_BUS_FMT_FIXED)
> +		return NULL;
> +
> +	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	*num_input_fmts = 1;
> +	input_fmts[0] = default_bus_format;

Why not 

	input_fmts[0] = MEDIA_BUS_FMT_RGB121212_1X36;

?

This way you could get rid of the default_bus_format variable.

> +	return input_fmts;
> +}
> +
>  static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>  				  struct drm_bridge_state *bridge_state,
>  				  struct drm_crtc_state *crtc_state,
> @@ -2142,6 +2166,7 @@ static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>  	.atomic_duplicate_state = cdns_mhdp_bridge_atomic_duplicate_state,
>  	.atomic_destroy_state = cdns_mhdp_bridge_atomic_destroy_state,
>  	.atomic_reset = cdns_mhdp_bridge_atomic_reset,
> +	.atomic_get_input_bus_fmts = cdns_mhdp_get_input_bus_fmts,
>  	.detect = cdns_mhdp_bridge_detect,
>  	.get_edid = cdns_mhdp_bridge_get_edid,
>  	.hpd_enable = cdns_mhdp_bridge_hpd_enable,

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 4/7] drm/bridge: mhdp8546: Set input_bus_flags from atomic_check

[Nikhil Devshatwar]

input_bus_flags are specified in drm_bridge_timings (legacy) as well
as drm_bridge_state->input_bus_cfg.flags

The flags from the timings will be deprecated. Bridges are supposed
to validate and set the bridge state flags from atomic_check.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 6 ++++++
 drivers/gpu/drm/bridge/ti-tfp410.c                  | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 2cd809eed827..9c17e4bb517e 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2121,6 +2121,12 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
 		return -EINVAL;
 	}
 
+	/*
+	 * There might be flags negotiation supported in future
+	 * Set the bus flags in atomic_check statically for now
+	 */
+	bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
+
 	mutex_unlock(&mhdp->link_mutex);
 	return 0;
 }
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index 4c536df003c8..9035d2145a28 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -245,6 +245,7 @@ int tfp410_atomic_check(struct drm_bridge *bridge,
 	 * Set the bus flags in atomic_check statically for now
 	 */
 	bridge_state->input_bus_cfg.flags = dvi->timings.input_bus_flags;
+	return 0;
 }
 
 static const struct drm_bridge_funcs tfp410_bridge_funcs = {
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 4/7] drm/bridge: mhdp8546: Set input_bus_flags from atomic_check

[Boris Brezillon]

On Tue, 1 Dec 2020 17:48:27 +0530
Nikhil Devshatwar <nikhil.nd@ti.com> wrote:

> input_bus_flags are specified in drm_bridge_timings (legacy) as well
> as drm_bridge_state->input_bus_cfg.flags
> 
> The flags from the timings will be deprecated. Bridges are supposed
> to validate and set the bridge state flags from atomic_check.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 6 ++++++
>  drivers/gpu/drm/bridge/ti-tfp410.c                  | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 2cd809eed827..9c17e4bb517e 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2121,6 +2121,12 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * There might be flags negotiation supported in future
> +	 * Set the bus flags in atomic_check statically for now
> +	 */
> +	bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
> +
>  	mutex_unlock(&mhdp->link_mutex);
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index 4c536df003c8..9035d2145a28 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -245,6 +245,7 @@ int tfp410_atomic_check(struct drm_bridge *bridge,
>  	 * Set the bus flags in atomic_check statically for now
>  	 */
>  	bridge_state->input_bus_cfg.flags = dvi->timings.input_bus_flags;
> +	return 0;

And here is the return statement that was missing in patch 2 :-).

>  }
>  
>  static const struct drm_bridge_funcs tfp410_bridge_funcs = {

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 4/7] drm/bridge: mhdp8546: Set input_bus_flags from atomic_check

[Nikhil Devshatwar]

On 11:32-20201204, Boris Brezillon wrote:
> On Tue, 1 Dec 2020 17:48:27 +0530
> Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
> 
> > input_bus_flags are specified in drm_bridge_timings (legacy) as well
> > as drm_bridge_state->input_bus_cfg.flags
> > 
> > The flags from the timings will be deprecated. Bridges are supposed
> > to validate and set the bridge state flags from atomic_check.
> > 
> > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > ---
> >  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 6 ++++++
> >  drivers/gpu/drm/bridge/ti-tfp410.c                  | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > index 2cd809eed827..9c17e4bb517e 100644
> > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > @@ -2121,6 +2121,12 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
> >  		return -EINVAL;
> >  	}
> >  
> > +	/*
> > +	 * There might be flags negotiation supported in future
> > +	 * Set the bus flags in atomic_check statically for now
> > +	 */
> > +	bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
> > +
> >  	mutex_unlock(&mhdp->link_mutex);
> >  	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> > index 4c536df003c8..9035d2145a28 100644
> > --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> > +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> > @@ -245,6 +245,7 @@ int tfp410_atomic_check(struct drm_bridge *bridge,
> >  	 * Set the bus flags in atomic_check statically for now
> >  	 */
> >  	bridge_state->input_bus_cfg.flags = dvi->timings.input_bus_flags;
> > +	return 0;
> 
> And here is the return statement that was missing in patch 2 :-).

Sorry. I guess I messed up while rebasing. Will fix this

Nikhil D

> 
> >  }
> >  
> >  static const struct drm_bridge_funcs tfp410_bridge_funcs = {
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 4/7] drm/bridge: mhdp8546: Set input_bus_flags from atomic_check

[Boris Brezillon]

On Tue, 1 Dec 2020 17:48:27 +0530
Nikhil Devshatwar <nikhil.nd@ti.com> wrote:

> input_bus_flags are specified in drm_bridge_timings (legacy) as well
> as drm_bridge_state->input_bus_cfg.flags
> 
> The flags from the timings will be deprecated. Bridges are supposed
> to validate and set the bridge state flags from atomic_check.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 6 ++++++
>  drivers/gpu/drm/bridge/ti-tfp410.c                  | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 2cd809eed827..9c17e4bb517e 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2121,6 +2121,12 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * There might be flags negotiation supported in future
> +	 * Set the bus flags in atomic_check statically for now
> +	 */
> +	bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;

I'd go even further and replace the timings field in
cdns_mhdp_platform_info by an input_bus_flags field, you'll then
have the following assignment here.

	if (mhdp->info)
		bridge_state->input_bus_cfg.flags = mhdp->info->input_bus_flags;

This way you no rely on the bridge->timings presence and can
get rid of the mhdp->bridge.timings assignment in the probe path.


> +
>  	mutex_unlock(&mhdp->link_mutex);
>  	return 0;
>  }

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 4/7] drm/bridge: mhdp8546: Set input_bus_flags from atomic_check

[Nikhil Devshatwar]

On 11:42-20201204, Boris Brezillon wrote:
> On Tue, 1 Dec 2020 17:48:27 +0530
> Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
> 
> > input_bus_flags are specified in drm_bridge_timings (legacy) as well
> > as drm_bridge_state->input_bus_cfg.flags
> > 
> > The flags from the timings will be deprecated. Bridges are supposed
> > to validate and set the bridge state flags from atomic_check.
> > 
> > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > ---
> >  drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 6 ++++++
> >  drivers/gpu/drm/bridge/ti-tfp410.c                  | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > index 2cd809eed827..9c17e4bb517e 100644
> > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> > @@ -2121,6 +2121,12 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
> >  		return -EINVAL;
> >  	}
> >  
> > +	/*
> > +	 * There might be flags negotiation supported in future
> > +	 * Set the bus flags in atomic_check statically for now
> > +	 */
> > +	bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
> 
> I'd go even further and replace the timings field in
> cdns_mhdp_platform_info by an input_bus_flags field, you'll then
> have the following assignment here.
> 
> 	if (mhdp->info)
> 		bridge_state->input_bus_cfg.flags = mhdp->info->input_bus_flags;
> 
> This way you no rely on the bridge->timings presence and can
> get rid of the mhdp->bridge.timings assignment in the probe path.
> 

Okay, I'll update this patch

> 
> > +
> >  	mutex_unlock(&mhdp->link_mutex);
> >  	return 0;
> >  }
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Nikhil Devshatwar]

Remove the old code to iterate over the bridge chain, as this is
already done by the framework.
The bridge state should have the negotiated bus format and flags.
Use these from the bridge's state.
If the bridge does not support format negotiation, error out
and fail.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---

Notes:
    changes from v3:
    * cosmetic updates
    changes from v2:
    * Remove the old code and use the flags from the bridge state

 drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++----------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index e278a9c89476..5deb8102e4d3 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -21,37 +21,29 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
 {
 	struct drm_device *ddev = encoder->dev;
 	struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
-	struct drm_display_info *di = &conn_state->connector->display_info;
+	struct drm_bridge_state *bstate;
 	struct drm_bridge *bridge;
-	bool bus_flags_set = false;
 
 	dev_dbg(ddev->dev, "%s\n", __func__);
 
-	/*
-	 * Take the bus_flags from the first bridge that defines
-	 * bridge timings, or from the connector's display_info if no
-	 * bridge defines the timings.
-	 */
-	drm_for_each_bridge_in_chain(encoder, bridge) {
-		if (!bridge->timings)
-			continue;
-
-		tcrtc_state->bus_flags = bridge->timings->input_bus_flags;
-		bus_flags_set = true;
-		break;
+	/* Copy the bus_format and flags from the first bridge's state */
+	bridge = drm_bridge_chain_get_first_bridge(encoder);
+	bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
+	if (!bstate) {
+		dev_err(ddev->dev, "Could not get the bridge state\n");
+		return -EINVAL;
 	}
 
-	if (!di->bus_formats || di->num_bus_formats == 0)  {
-		dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
-			__func__);
+	tcrtc_state->bus_format = bstate->input_bus_cfg.format;
+	tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;
+
+	if (tcrtc_state->bus_format == 0 ||
+	    tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {
+
+		dev_err(ddev->dev, "Bridge connected to the encoder did not specify media bus format\n");
 		return -EINVAL;
 	}
 
-	// XXX any cleaner way to set bus format and flags?
-	tcrtc_state->bus_format = di->bus_formats[0];
-	if (!bus_flags_set)
-		tcrtc_state->bus_flags = di->bus_flags;
-
 	return 0;
 }
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Boris Brezillon]

On Tue, 1 Dec 2020 17:48:28 +0530
Nikhil Devshatwar <nikhil.nd@ti.com> wrote:

> Remove the old code to iterate over the bridge chain, as this is
> already done by the framework.
> The bridge state should have the negotiated bus format and flags.
> Use these from the bridge's state.
> If the bridge does not support format negotiation, error out
> and fail.

That'd be even better if you implement the bridge interface instead of
the encoder one so we can get rid of the encoder_{helper}_funcs and use
drm_simple_encoder_init().

> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> 
> Notes:
>     changes from v3:
>     * cosmetic updates
>     changes from v2:
>     * Remove the old code and use the flags from the bridge state
> 
>  drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++----------------
>  1 file changed, 14 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> index e278a9c89476..5deb8102e4d3 100644
> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> @@ -21,37 +21,29 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
>  {
>  	struct drm_device *ddev = encoder->dev;
>  	struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
> -	struct drm_display_info *di = &conn_state->connector->display_info;
> +	struct drm_bridge_state *bstate;
>  	struct drm_bridge *bridge;
> -	bool bus_flags_set = false;
>  
>  	dev_dbg(ddev->dev, "%s\n", __func__);
>  
> -	/*
> -	 * Take the bus_flags from the first bridge that defines
> -	 * bridge timings, or from the connector's display_info if no
> -	 * bridge defines the timings.
> -	 */
> -	drm_for_each_bridge_in_chain(encoder, bridge) {
> -		if (!bridge->timings)
> -			continue;
> -
> -		tcrtc_state->bus_flags = bridge->timings->input_bus_flags;
> -		bus_flags_set = true;
> -		break;
> +	/* Copy the bus_format and flags from the first bridge's state */
> +	bridge = drm_bridge_chain_get_first_bridge(encoder);
> +	bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
> +	if (!bstate) {
> +		dev_err(ddev->dev, "Could not get the bridge state\n");
> +		return -EINVAL;
>  	}
>  
> -	if (!di->bus_formats || di->num_bus_formats == 0)  {
> -		dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
> -			__func__);
> +	tcrtc_state->bus_format = bstate->input_bus_cfg.format;
> +	tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;
> +
> +	if (tcrtc_state->bus_format == 0 ||
> +	    tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {
> +
> +		dev_err(ddev->dev, "Bridge connected to the encoder did not specify media bus format\n");
>  		return -EINVAL;
>  	}
>  
> -	// XXX any cleaner way to set bus format and flags?
> -	tcrtc_state->bus_format = di->bus_formats[0];
> -	if (!bus_flags_set)
> -		tcrtc_state->bus_flags = di->bus_flags;
> -
>  	return 0;
>  }
>  

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Tomi Valkeinen]

Hi Boris,

On 04/12/2020 12:50, Boris Brezillon wrote:
> On Tue, 1 Dec 2020 17:48:28 +0530
> Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
> 
>> Remove the old code to iterate over the bridge chain, as this is
>> already done by the framework.
>> The bridge state should have the negotiated bus format and flags.
>> Use these from the bridge's state.
>> If the bridge does not support format negotiation, error out
>> and fail.
> 
> That'd be even better if you implement the bridge interface instead of
> the encoder one so we can get rid of the encoder_{helper}_funcs and use
> drm_simple_encoder_init().

I'm a bit confused here. What should be the design here...

These formats need to be handled by the crtc (well, the display controller, which is modeled as the
crtc). Should we have a tidss_crtc.c file, which implements a crtc, a simple encoder and a bridge?
And just consider those three DRM components as different API parts of the display controller?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Boris Brezillon]

On Fri, 4 Dec 2020 12:56:27 +0200
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> Hi Boris,
> 
> On 04/12/2020 12:50, Boris Brezillon wrote:
> > On Tue, 1 Dec 2020 17:48:28 +0530
> > Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
> >   
> >> Remove the old code to iterate over the bridge chain, as this is
> >> already done by the framework.
> >> The bridge state should have the negotiated bus format and flags.
> >> Use these from the bridge's state.
> >> If the bridge does not support format negotiation, error out
> >> and fail.  
> > 
> > That'd be even better if you implement the bridge interface instead of
> > the encoder one so we can get rid of the encoder_{helper}_funcs and use
> > drm_simple_encoder_init().  
> 
> I'm a bit confused here. What should be the design here...
> 
> These formats need to be handled by the crtc (well, the display controller, which is modeled as the
> crtc). Should we have a tidss_crtc.c file, which implements a crtc, a simple encoder and a bridge?
> And just consider those three DRM components as different API parts of the display controller?

The idea is to hide the encoder abstraction from drivers as much as we
can. We have to keep the encoder concept because that's what we expose
to userspace, but drivers shouldn't have to worry about the distinction
between the first bridge in the chain (what we currently call encoders)
and other bridges. The bridge interface provides pretty much the same
functionality, so all you need to do is turn your encoder driver into a
bridge driver (see what we did for
drivers/gpu/drm/imx/parallel-display.c), the only particularity here is
that the bridge knows it's the first in the chain, and has access to
the CRTC it's directly connected to.

IMHO, the less interfaces we keep the clearer it gets for our users.
Getting rid of the encoder_{helper_}funcs in favor or bridge_ops would
clarify the fact that any kind of encoder, no matter if it's the first
in the chain or not, should be represented as a bridge object.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Tomi Valkeinen]

On 04/12/2020 13:12, Boris Brezillon wrote:

>>> That'd be even better if you implement the bridge interface instead of
>>> the encoder one so we can get rid of the encoder_{helper}_funcs and use
>>> drm_simple_encoder_init().  
>>
>> I'm a bit confused here. What should be the design here...
>>
>> These formats need to be handled by the crtc (well, the display controller, which is modeled as the
>> crtc). Should we have a tidss_crtc.c file, which implements a crtc, a simple encoder and a bridge?
>> And just consider those three DRM components as different API parts of the display controller?
> 
> The idea is to hide the encoder abstraction from drivers as much as we
> can. We have to keep the encoder concept because that's what we expose
> to userspace, but drivers shouldn't have to worry about the distinction
> between the first bridge in the chain (what we currently call encoders)
> and other bridges. The bridge interface provides pretty much the same
> functionality, so all you need to do is turn your encoder driver into a
> bridge driver (see what we did for
> drivers/gpu/drm/imx/parallel-display.c), the only particularity here is
> that the bridge knows it's the first in the chain, and has access to
> the CRTC it's directly connected to.

With a quick look, the imx parallel-display.c seems to be really part of the crtc. Shouldn't we then
take one more step forward, and just combine the crtc, encoder and bridge (somehow)? That's kind of
what parallel-display.c is doing, isn't it? It's directly poking the crtc state, but the code just
happens to be in a separate file from the crtc.

Thinking about the TI HW, we have a bunch of internal IPs which are clearly bridges: HDMI, DSI,
which get the pixel data from the display controller, and they have their own registers, so clearly
independent bridges.

Then we have MIPI DPI, which doesn't really have its own registers, as it's just plain parallel RGB,
the same as what is sent to e.g. HDMI and DSI bridges. However, there might be some muxes or
regulators to set up to get the external signals working. So a bridge would be ok here too to handle
that external side.

But in all the above cases, we have the display controller (crtc), which in all the cases needs to
do e.g. pixel/bus format configuration. So why add direct crtc poking into the first bridges (HDMI,
DSI, DPI), when we could just model the crtc as a bridge, and thus the first bridges wouldn't need
to touch the crtc.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Boris Brezillon]

On Fri, 4 Dec 2020 13:47:05 +0200
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> On 04/12/2020 13:12, Boris Brezillon wrote:
> 
> >>> That'd be even better if you implement the bridge interface instead of
> >>> the encoder one so we can get rid of the encoder_{helper}_funcs and use
> >>> drm_simple_encoder_init().    
> >>
> >> I'm a bit confused here. What should be the design here...
> >>
> >> These formats need to be handled by the crtc (well, the display controller, which is modeled as the
> >> crtc). Should we have a tidss_crtc.c file, which implements a crtc, a simple encoder and a bridge?
> >> And just consider those three DRM components as different API parts of the display controller?  
> > 
> > The idea is to hide the encoder abstraction from drivers as much as we
> > can. We have to keep the encoder concept because that's what we expose
> > to userspace, but drivers shouldn't have to worry about the distinction
> > between the first bridge in the chain (what we currently call encoders)
> > and other bridges. The bridge interface provides pretty much the same
> > functionality, so all you need to do is turn your encoder driver into a
> > bridge driver (see what we did for
> > drivers/gpu/drm/imx/parallel-display.c), the only particularity here is
> > that the bridge knows it's the first in the chain, and has access to
> > the CRTC it's directly connected to.  
> 
> With a quick look, the imx parallel-display.c seems to be really part of the crtc. Shouldn't we then
> take one more step forward, and just combine the crtc, encoder and bridge (somehow)? That's kind of
> what parallel-display.c is doing, isn't it? It's directly poking the crtc state, but the code just
> happens to be in a separate file from the crtc.

Right. If we want to keep the code split, the logic should probably be
reversed, with the CRTC driver checking the first bridge state to setup
its internal state. Those DPI encoders are always a bit special, since
they tend to be directly embedded in the block responsible for timing
control (what we call CRTCs), so you're right, maybe we should model
that as a CRTC+bridge pair.

> 
> Thinking about the TI HW, we have a bunch of internal IPs which are clearly bridges: HDMI, DSI,
> which get the pixel data from the display controller, and they have their own registers, so clearly
> independent bridges.
> 
> Then we have MIPI DPI, which doesn't really have its own registers, as it's just plain parallel RGB,
> the same as what is sent to e.g. HDMI and DSI bridges.

I still consider that one a bridge, even if the translation is almost
transparent from a HW PoV.

> However, there might be some muxes or
> regulators to set up to get the external signals working. So a bridge would be ok here too to handle
> that external side.

Exactly.

> 
> But in all the above cases, we have the display controller (crtc), which in all the cases needs to
> do e.g. pixel/bus format configuration. So why add direct crtc poking into the first bridges (HDMI,
> DSI, DPI), when we could just model the crtc as a bridge, and thus the first bridges wouldn't need
> to touch the crtc.

Yes, that's an option. I mean, you can already modify your CRTC
logic to implement the bridge and CRTC interface and have your
driver-specific CRTC object embed both a drm_crtc and a drm_bridge.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Tomi Valkeinen]

On 04/12/2020 15:54, Boris Brezillon wrote:
> On Fri, 4 Dec 2020 13:47:05 +0200
> Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> 
>> On 04/12/2020 13:12, Boris Brezillon wrote:
>>
>>>>> That'd be even better if you implement the bridge interface instead of
>>>>> the encoder one so we can get rid of the encoder_{helper}_funcs and use
>>>>> drm_simple_encoder_init().    
>>>>
>>>> I'm a bit confused here. What should be the design here...
>>>>
>>>> These formats need to be handled by the crtc (well, the display controller, which is modeled as the
>>>> crtc). Should we have a tidss_crtc.c file, which implements a crtc, a simple encoder and a bridge?
>>>> And just consider those three DRM components as different API parts of the display controller?  
>>>
>>> The idea is to hide the encoder abstraction from drivers as much as we
>>> can. We have to keep the encoder concept because that's what we expose
>>> to userspace, but drivers shouldn't have to worry about the distinction
>>> between the first bridge in the chain (what we currently call encoders)
>>> and other bridges. The bridge interface provides pretty much the same
>>> functionality, so all you need to do is turn your encoder driver into a
>>> bridge driver (see what we did for
>>> drivers/gpu/drm/imx/parallel-display.c), the only particularity here is
>>> that the bridge knows it's the first in the chain, and has access to
>>> the CRTC it's directly connected to.  
>>
>> With a quick look, the imx parallel-display.c seems to be really part of the crtc. Shouldn't we then
>> take one more step forward, and just combine the crtc, encoder and bridge (somehow)? That's kind of
>> what parallel-display.c is doing, isn't it? It's directly poking the crtc state, but the code just
>> happens to be in a separate file from the crtc.
> 
> Right. If we want to keep the code split, the logic should probably be
> reversed, with the CRTC driver checking the first bridge state to setup
> its internal state. Those DPI encoders are always a bit special, since
> they tend to be directly embedded in the block responsible for timing
> control (what we call CRTCs), so you're right, maybe we should model
> that as a CRTC+bridge pair.
> 
>>
>> Thinking about the TI HW, we have a bunch of internal IPs which are clearly bridges: HDMI, DSI,
>> which get the pixel data from the display controller, and they have their own registers, so clearly
>> independent bridges.
>>
>> Then we have MIPI DPI, which doesn't really have its own registers, as it's just plain parallel RGB,
>> the same as what is sent to e.g. HDMI and DSI bridges.
> 
> I still consider that one a bridge, even if the translation is almost
> transparent from a HW PoV.
> 
>> However, there might be some muxes or
>> regulators to set up to get the external signals working. So a bridge would be ok here too to handle
>> that external side.
> 
> Exactly.
> 
>>
>> But in all the above cases, we have the display controller (crtc), which in all the cases needs to
>> do e.g. pixel/bus format configuration. So why add direct crtc poking into the first bridges (HDMI,
>> DSI, DPI), when we could just model the crtc as a bridge, and thus the first bridges wouldn't need
>> to touch the crtc.
> 
> Yes, that's an option. I mean, you can already modify your CRTC
> logic to implement the bridge and CRTC interface and have your
> driver-specific CRTC object embed both a drm_crtc and a drm_bridge.

Alright, thanks for the clarifications!

I think the best first step here is what you proposed, use the bridge interface and
drm_simple_encoder_init(), as that should solve the issue at hand quite neatly.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Francesco Dolcini]

Hello,
chiming in in this *old* email thread.

Adding in copy a few more *@ti.com people recently involved in other tidss
changes [1]


[1] https://lore.kernel.org/all/20230125113529.13952-1-a-bhatia1@ti.com/


On Fri, Dec 04, 2020 at 11:50:30AM +0100, Boris Brezillon wrote:
> On Tue, 1 Dec 2020 17:48:28 +0530
> Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
> 
> > Remove the old code to iterate over the bridge chain, as this is
> > already done by the framework.
> > The bridge state should have the negotiated bus format and flags.
> > Use these from the bridge's state.
> > If the bridge does not support format negotiation, error out
> > and fail.
> 
> That'd be even better if you implement the bridge interface instead of
> the encoder one so we can get rid of the encoder_{helper}_funcs and use
> drm_simple_encoder_init().

Did anything happened on this old discussion? I was working on a very
similar change and after a while I realized about this thread.

Thanks,
Francesco

Re: [PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Aradhya Bhatia]

Hi Francesco,

On 30-Mar-23 22:47, Francesco Dolcini wrote:
> Hello,
> chiming in in this *old* email thread.
> 
> Adding in copy a few more *@ti.com people recently involved in other tidss
> changes [1]
> 
> 
> [1] https://lore.kernel.org/all/20230125113529.13952-1-a-bhatia1@ti.com/
> 
> 
> On Fri, Dec 04, 2020 at 11:50:30AM +0100, Boris Brezillon wrote:
>> On Tue, 1 Dec 2020 17:48:28 +0530
>> Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
>>
>>> Remove the old code to iterate over the bridge chain, as this is
>>> already done by the framework.
>>> The bridge state should have the negotiated bus format and flags.
>>> Use these from the bridge's state.
>>> If the bridge does not support format negotiation, error out
>>> and fail.
>>
>> That'd be even better if you implement the bridge interface instead of
>> the encoder one so we can get rid of the encoder_{helper}_funcs and use
>> drm_simple_encoder_init().
> 
> Did anything happened on this old discussion? I was working on a very
> similar change and after a while I realized about this thread.
> 
Nikhil has moved on from TI.

I will be taking up this patch series and implement the changes
requested.


Boris, Tomi

Are the changes discussed on this patch still relevant for the display
controller driver to support DRM_BRIDGE_ATTACH_NO_CONNECTOR?

Or are there some new changes in the framework that I should be looking
at?

Regards
Aradhya

Re: [PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Francesco Dolcini]

Hello Aradhya,

On Fri, Apr 14, 2023 at 01:19:38PM +0530, Aradhya Bhatia wrote:
> On 30-Mar-23 22:47, Francesco Dolcini wrote:
> > Hello,
> > chiming in in this *old* email thread.
> > 
> > Adding in copy a few more *@ti.com people recently involved in other tidss
> > changes [1]
> > 
> > 
> > [1] https://lore.kernel.org/all/20230125113529.13952-1-a-bhatia1@ti.com/
> > 
> > 
> > On Fri, Dec 04, 2020 at 11:50:30AM +0100, Boris Brezillon wrote:
> >> On Tue, 1 Dec 2020 17:48:28 +0530
> >> Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
> >>
> >>> Remove the old code to iterate over the bridge chain, as this is
> >>> already done by the framework.
> >>> The bridge state should have the negotiated bus format and flags.
> >>> Use these from the bridge's state.
> >>> If the bridge does not support format negotiation, error out
> >>> and fail.
> >>
> >> That'd be even better if you implement the bridge interface instead of
> >> the encoder one so we can get rid of the encoder_{helper}_funcs and use
> >> drm_simple_encoder_init().
> > 
> > Did anything happened on this old discussion? I was working on a very
> > similar change and after a while I realized about this thread.
> > 
> Nikhil has moved on from TI.
> 
> I will be taking up this patch series and implement the changes
> requested.
Great!

What I was working on is really about having the media bus format taken
from the closest bridge, this is really required for proper functionality
when the display is connected through a bridge.

This [1] is what I was working on before realizing about this specific
patch here, in case you want to have a look.

Please keep me in CC, I can test/review patches.

Francesco

[1] https://github.com/dolcini/linux/commit/150b1390bd4122a77c873d87bf506024f9775755

Re: [PATCH v4 5/7] drm/tidss: Set bus_format correctly from bridge/connector

[Aradhya Bhatia]

Hi Francesco,

On 14-Apr-23 14:00, Francesco Dolcini wrote:
> Hello Aradhya,
> 
> On Fri, Apr 14, 2023 at 01:19:38PM +0530, Aradhya Bhatia wrote:
>> On 30-Mar-23 22:47, Francesco Dolcini wrote:
>>> Hello,
>>> chiming in in this *old* email thread.
>>>
>>> Adding in copy a few more *@ti.com people recently involved in other tidss
>>> changes [1]
>>>
>>>
>>> [1] https://lore.kernel.org/all/20230125113529.13952-1-a-bhatia1@ti.com/
>>>
>>>
>>> On Fri, Dec 04, 2020 at 11:50:30AM +0100, Boris Brezillon wrote:
>>>> On Tue, 1 Dec 2020 17:48:28 +0530
>>>> Nikhil Devshatwar <nikhil.nd@ti.com> wrote:
>>>>
>>>>> Remove the old code to iterate over the bridge chain, as this is
>>>>> already done by the framework.
>>>>> The bridge state should have the negotiated bus format and flags.
>>>>> Use these from the bridge's state.
>>>>> If the bridge does not support format negotiation, error out
>>>>> and fail.
>>>>
>>>> That'd be even better if you implement the bridge interface instead of
>>>> the encoder one so we can get rid of the encoder_{helper}_funcs and use
>>>> drm_simple_encoder_init().
>>>
>>> Did anything happened on this old discussion? I was working on a very
>>> similar change and after a while I realized about this thread.
>>>
>> Nikhil has moved on from TI.
>>
>> I will be taking up this patch series and implement the changes
>> requested.
> Great!
> 
> What I was working on is really about having the media bus format taken
> from the closest bridge, this is really required for proper functionality
> when the display is connected through a bridge.

I agree with you there. The current code will not work well when the
media bus format required by the final sink is not same as the one tidss
needs to output.

> 
> This [1] is what I was working on before realizing about this specific
> patch here, in case you want to have a look.
> 
> Please keep me in CC, I can test/review patches.

Thank you! Will do. =)


Regards
Aradhya

[PATCH v4 6/7] drm/tidss: Move to newer connector model

[Nikhil Devshatwar]

To be able to support connector operations across multiple
bridges, it is recommended that the connector should be
created by the SoC driver instead of the bridges.

Modify the tidss modesetting initialization sequence to
create the connector and attach bridges with flag
DRM_BRIDGE_ATTACH_NO_CONNECTOR

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

Notes:
    changes from v1:
    * Add error handling

 drivers/gpu/drm/tidss/tidss_drv.h |  3 +++
 drivers/gpu/drm/tidss/tidss_kms.c | 19 ++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index 7de4bba52e6f..cfbf85a4d92b 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -27,6 +27,9 @@ struct tidss_device {
 	unsigned int num_planes;
 	struct drm_plane *planes[TIDSS_MAX_PLANES];
 
+	unsigned int num_connectors;
+	struct drm_connector *connectors[TIDSS_MAX_PORTS];
+
 	spinlock_t wait_lock;	/* protects the irq masks */
 	dispc_irq_t irq_mask;	/* enabled irqs in addition to wait_list */
 };
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index 09485c7f0d6f..1f5ae153b114 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -7,6 +7,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_fb_helper.h>
@@ -192,6 +193,7 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 	for (i = 0; i < num_pipes; ++i) {
 		struct tidss_plane *tplane;
 		struct tidss_crtc *tcrtc;
+		struct drm_connector *connector;
 		struct drm_encoder *enc;
 		u32 hw_plane_id = feat->vid_order[tidss->num_planes];
 		int ret;
@@ -222,11 +224,26 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
 			return PTR_ERR(enc);
 		}
 
-		ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0);
+		ret = drm_bridge_attach(enc, pipes[i].bridge, NULL,
+					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 		if (ret) {
 			dev_err(tidss->dev, "bridge attach failed: %d\n", ret);
 			return ret;
 		}
+
+		connector = drm_bridge_connector_init(&tidss->ddev, enc);
+		if (IS_ERR(connector)) {
+			dev_err(tidss->dev, "bridge_connector create failed\n");
+			return PTR_ERR(connector);
+		}
+
+		tidss->connectors[tidss->num_connectors++] = connector;
+
+		ret = drm_connector_attach_encoder(connector, enc);
+		if (ret) {
+			dev_err(tidss->dev, "attaching encoder to connector failed\n");
+			return ret;
+		}
 	}
 
 	/* create overlay planes of the leftover planes */
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 7/7] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable

[Nikhil Devshatwar]

When removing the tidss driver, there is a warning reported by
kernel about an unhandled interrupt for mhdp driver.

[   43.238895] irq 31: nobody cared (try booting with the "irqpoll" option)
... [snipped backtrace]
[   43.330735] handlers:
[   43.333020] [<000000005367c4f9>] irq_default_primary_handler threaded [<000000007e02b601>]
cdns_mhdp_irq_handler [cdns_mhdp8546]
[   43.344607] Disabling IRQ #31

This happens because as part of cdns_mhdp_bridge_hpd_disable, driver tries
to disable the interrupts. While disabling the SW_EVENT interrupts,
it accidentally enables the MBOX interrupts, which are not handled by
the driver.

Fix this with a read-modify-write to update only required bits.
Use the enable / disable function as required in other places.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
Reviewed-by: Swapnil Jakhade <sjakhade@cadence.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---

Notes:
    changes from v2:
    * Fix the interrupt enable logic at other places in code
    * Reorder functions so that they can be used earlier in the program

 .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 44 +++++++++----------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 9c17e4bb517e..d0ed950f4f87 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -52,6 +52,26 @@
 
 #include "cdns-mhdp8546-j721e.h"
 
+static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
+
+	/* Enable SW event interrupts */
+	if (mhdp->bridge_attached)
+		writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
+		       ~CDNS_APB_INT_MASK_SW_EVENT_INT,
+		       mhdp->regs + CDNS_APB_INT_MASK);
+}
+
+static void cdns_mhdp_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
+
+	writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
+	       CDNS_APB_INT_MASK_SW_EVENT_INT,
+	       mhdp->regs + CDNS_APB_INT_MASK);
+}
+
 static int cdns_mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
 {
 	int ret, empty;
@@ -747,9 +767,7 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw,
 	 * MHDP_HW_STOPPED happens only due to driver removal when
 	 * bridge should already be detached.
 	 */
-	if (mhdp->bridge_attached)
-		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
-		       mhdp->regs + CDNS_APB_INT_MASK);
+	cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);
 
 	spin_unlock(&mhdp->start_lock);
 
@@ -1689,8 +1707,7 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,
 
 	/* Enable SW event interrupts */
 	if (hw_ready)
-		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
-		       mhdp->regs + CDNS_APB_INT_MASK);
+		cdns_mhdp_bridge_hpd_enable(bridge);
 
 	return 0;
 }
@@ -2146,23 +2163,6 @@ static struct edid *cdns_mhdp_bridge_get_edid(struct drm_bridge *bridge,
 	return cdns_mhdp_get_edid(mhdp, connector);
 }
 
-static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
-{
-	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
-
-	/* Enable SW event interrupts */
-	if (mhdp->bridge_attached)
-		writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
-		       mhdp->regs + CDNS_APB_INT_MASK);
-}
-
-static void cdns_mhdp_bridge_hpd_disable(struct drm_bridge *bridge)
-{
-	struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
-
-	writel(CDNS_APB_INT_MASK_SW_EVENT_INT, mhdp->regs + CDNS_APB_INT_MASK);
-}
-
 static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
 	.atomic_enable = cdns_mhdp_atomic_enable,
 	.atomic_disable = cdns_mhdp_atomic_disable,
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel