Re: [Freedreno] [PATCH v3 3/3] drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler()

2023-07-21 Thread Liviu Dudau 
Hi Gaosheng,

On Fri, Jul 14, 2023 at 09:48:20AM +0800, Gaosheng Cui wrote:
> The komeda_pipeline_get_state() returns an ERR_PTR() on failure, we should
> use IS_ERR() to check the return value.
> 
> Fixes: 502932a03fce ("drm/komeda: Add the initial scaler support for CORE")
> Signed-off-by: Gaosheng Cui 
> Reviewed-by: Liviu Dudau 
> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
> b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index 3276a3e82c62..e9c92439398d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -259,7 +259,7 @@ komeda_component_get_avail_scaler(struct komeda_component 
> *c,
>   u32 avail_scalers;
>  
>   pipe_st = komeda_pipeline_get_state(c->pipeline, state);
> - if (!pipe_st)
> + if (IS_ERR(pipe_st))

If you're going to update the other patches to use IS_ERR_OR_NULL() please do so
here too. You can keep my R-b for that change.

Best regards,
Liviu

>   return NULL;
>  
>   avail_scalers = (pipe_st->active_comps & KOMEDA_PIPELINE_SCALERS) ^
> -- 
> 2.25.1
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

Re: [Freedreno] [PATCH] drm: Explicitly include correct DT includes

2023-07-17 Thread Liviu Dudau 
On Fri, Jul 14, 2023 at 11:45:34AM -0600, Rob Herring wrote:
> The DT of_device.h and of_platform.h date back to the separate
> of_platform_bus_type before it as merged into the regular platform bus.
> As part of that merge prepping Arm DT support 13 years ago, they
> "temporarily" include each other. They also include platform_device.h
> and of.h. As a result, there's a pretty much random mix of those include
> files used throughout the tree. In order to detangle these headers and
> replace the implicit includes with struct declarations, users need to
> explicitly include the correct includes.
> 
> Signed-off-by: Rob Herring 
> ---
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c 
> b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> index cc7664c95a54..14ee79becacb 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> @@ -6,7 +6,7 @@
>   */
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c 
> b/drivers/gpu/drm/arm/malidp_drv.c
> index c03cfd57b752..a5a9534d4353 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -12,6 +12,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>

For the komeda and malidp drivers:

Acked-by: Liviu Dudau 

Best regards,
Liviu


> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c 
> b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> index 99964f5a5457..2a6b91f752cb 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> @@ -7,7 +7,6 @@
>  
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 2254457ab5d0..b9957da0f55a 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -9,7 +9,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  
>  #include 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c 
> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index f50d65f54314..7457d38622b0 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -14,8 +14,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c 
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index f6822dfa3805..4aff817f82ce 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -29,7 +29,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c 
> b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index 8bfce21d6b90..d205e755e524 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -17,7 +17,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  
> diff --git a/drivers/gpu/drm/bridge/display-connector.c 
> b/drivers/gpu/drm/bridge/display-connector.c
> index f7f436cf96e0..08bd5695ddae 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -10,7 +10,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c 
> b/drivers/gpu/drm/bridge/fsl-ldb.c
> index b8e52156b07a..0e4bac7dd04f 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -8,7 +8,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qm-ldb.c 
> b/drivers/gpu/drm/bridge/imx/imx8qm-ldb.c
> index 386032a02599..21471a9a28b2 100644
> --- a/drivers/gpu/drm/bridge/imx/imx8qm-ldb.c
> +++ b/drivers/gpu/drm/bridge/imx/imx8qm-ldb.c
> @@ -9,9 +9,9 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> diff --git a/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c 
> b/drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c
> index c806576b1e22..7984da9c0a35 100644
> --- a/drivers/gpu/drm

Re: [Freedreno] [PATCH v2 3/3] drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler()

2023-07-13 Thread Liviu Dudau 
On Thu, Jul 13, 2023 at 05:51:34PM +0800, cuigaosheng wrote:
> Thanks for taking time to review this patch.
> 
> Maybe we can submit another separate patch to fix ERR_CAST(st), because I'm 
> not
> sure which commit should be used as a fixes-tag.
> 
> Also, Maybe we should fix ERR_CAST(st) in 
> komeda_pipeline_get_state_and_set_crtc()
> and komeda_component_get_state_and_set_user(), please make another separate 
> patch.

Yeah, you're right, there are other places where this needs to be fixed.

I will add this to my list of ToDos.

Best regards,
Liviu

> 
> Let me know if there's anything I can do, thanks for your work again!
> 
> Gaosheng,
> 
> On 2023/7/13 16:54, Liviu Dudau wrote:
> > Hello,
> > 
> > On Thu, Jul 13, 2023 at 10:05:56AM +0800, Gaosheng Cui wrote:
> > > The komeda_pipeline_get_state() returns an ERR_PTR() on failure, we should
> > > use IS_ERR() to check the return value.
> > While reviewing the change I've realised that 
> > komeda_pipeline_get_state_and_set_crtc()
> > is also mishandling the return value from komeda_pipeline_get_state(). If 
> > IS_ERR(st) is
> > true it should use return ERR_CAST(st), following the same pattern as 
> > komeda_pipeline_get_state().
> > 
> > If you don't want to update this patch I can send a separate patch. 
> > Otherwise,
> > the change looks good to me.
> > 
> > Reviewed-by: Liviu Dudau 
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > Fixes: 502932a03fce ("drm/komeda: Add the initial scaler support for 
> > > CORE")
> > > Signed-off-by: Gaosheng Cui 
> > > ---
> > >   drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
> > > b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > > index 3276a3e82c62..e9c92439398d 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > > @@ -259,7 +259,7 @@ komeda_component_get_avail_scaler(struct 
> > > komeda_component *c,
> > >   u32 avail_scalers;
> > >   pipe_st = komeda_pipeline_get_state(c->pipeline, state);
> > > - if (!pipe_st)
> > > + if (IS_ERR(pipe_st))
> > >   return NULL;
> > >   avail_scalers = (pipe_st->active_comps & 
> > > KOMEDA_PIPELINE_SCALERS) ^
> > > -- 
> > > 2.25.1
> > > 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

Re: [Freedreno] [PATCH v2 3/3] drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler()

2023-07-13 Thread Liviu Dudau 
Hello,

On Thu, Jul 13, 2023 at 10:05:56AM +0800, Gaosheng Cui wrote:
> The komeda_pipeline_get_state() returns an ERR_PTR() on failure, we should
> use IS_ERR() to check the return value.

While reviewing the change I've realised that 
komeda_pipeline_get_state_and_set_crtc()
is also mishandling the return value from komeda_pipeline_get_state(). If 
IS_ERR(st) is
true it should use return ERR_CAST(st), following the same pattern as 
komeda_pipeline_get_state().

If you don't want to update this patch I can send a separate patch. Otherwise,
the change looks good to me.

Reviewed-by: Liviu Dudau 

Best regards,
Liviu


> 
> Fixes: 502932a03fce ("drm/komeda: Add the initial scaler support for CORE")
> Signed-off-by: Gaosheng Cui 
> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
> b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index 3276a3e82c62..e9c92439398d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -259,7 +259,7 @@ komeda_component_get_avail_scaler(struct komeda_component 
> *c,
>   u32 avail_scalers;
>  
>   pipe_st = komeda_pipeline_get_state(c->pipeline, state);
> - if (!pipe_st)
> + if (IS_ERR(pipe_st))
>   return NULL;
>  
>   avail_scalers = (pipe_st->active_comps & KOMEDA_PIPELINE_SCALERS) ^
> -- 
> 2.25.1
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

Re: [Freedreno] [PATCH v3 3/3] drm: Convert users of drm_of_component_match_add to component_match_add_of

2023-01-20 Thread Liviu Dudau 
On Thu, Jan 19, 2023 at 02:10:39PM -0500, Sean Anderson wrote:
> Every user of this function either uses component_compare_of or
> something equivalent. Most of them immediately put the device node as
> well. Convert these users to component_match_add_of and remove
> drm_of_component_match_add.
> 
> Signed-off-by: Sean Anderson 
> Acked-by: Jyri Sarha 
> Tested-by: Jyri Sarha 
> ---
> 
> (no changes since v1)
> 
>  .../gpu/drm/arm/display/komeda/komeda_drv.c   |  6 ++--
>  drivers/gpu/drm/arm/hdlcd_drv.c   |  9 +-
>  drivers/gpu/drm/arm/malidp_drv.c  | 11 +--
>  drivers/gpu/drm/armada/armada_drv.c   | 10 ---
>  drivers/gpu/drm/drm_of.c  | 29 +++
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c |  4 +--
>  .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c   |  3 +-
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c |  3 +-
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c|  4 +--
>  drivers/gpu/drm/msm/msm_drv.c | 14 -
>  drivers/gpu/drm/sti/sti_drv.c |  3 +-
>  drivers/gpu/drm/sun4i/sun4i_drv.c |  3 +-
>  drivers/gpu/drm/tilcdc/tilcdc_external.c  | 10 ++-
>  include/drm/drm_of.h  | 12 
>  14 files changed, 33 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c 
> b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index 3f4e719eebd8..e3bfc72c378f 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -103,10 +103,8 @@ static void komeda_add_slave(struct device *master,
>   struct device_node *remote;
>  
>   remote = of_graph_get_remote_node(np, port, endpoint);
> - if (remote) {
> - drm_of_component_match_add(master, match, component_compare_of, 
> remote);
> - of_node_put(remote);
> - }
> + if (remote)
> + component_match_add_of(master, match, remote);
>  }
>  
>  static int komeda_platform_probe(struct platform_device *pdev)
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index e3507dd6f82a..5f760bb66af4 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -347,11 +347,6 @@ static const struct component_master_ops 
> hdlcd_master_ops = {
>   .unbind = hdlcd_drm_unbind,
>  };
>  
> -static int compare_dev(struct device *dev, void *data)
> -{
> - return dev->of_node == data;
> -}
> -
>  static int hdlcd_probe(struct platform_device *pdev)
>  {
>   struct device_node *port;
> @@ -362,9 +357,7 @@ static int hdlcd_probe(struct platform_device *pdev)
>   if (!port)
>   return -ENODEV;
>  
> - drm_of_component_match_add(>dev, , compare_dev, port);
> - of_node_put(port);
> -
> + component_match_add_of(>dev, , port);
>   return component_master_add_with_match(>dev, _master_ops,
>  match);
>  }
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c 
> b/drivers/gpu/drm/arm/malidp_drv.c
> index 589c1c66a6dc..3a49c29ba5b8 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -909,13 +909,6 @@ static const struct component_master_ops 
> malidp_master_ops = {
>   .unbind = malidp_unbind,
>  };
>  
> -static int malidp_compare_dev(struct device *dev, void *data)
> -{
> - struct device_node *np = data;
> -
> - return dev->of_node == np;
> -}
> -
>  static int malidp_platform_probe(struct platform_device *pdev)
>  {
>   struct device_node *port;
> @@ -929,9 +922,7 @@ static int malidp_platform_probe(struct platform_device 
> *pdev)
>   if (!port)
>   return -ENODEV;
>  
> - drm_of_component_match_add(>dev, , malidp_compare_dev,
> -port);
> - of_node_put(port);
> + component_match_add_of(>dev, , port);
>   return component_master_add_with_match(>dev, _master_ops,
>  match);
>  }

For komeda, mali_dp and hdlcd: Acked-by: Liviu Dudau 

Best regards,
Liviu


> diff --git a/drivers/gpu/drm/armada/armada_drv.c 
> b/drivers/gpu/drm/armada/armada_drv.c
> index 0643887800b4..c0211ad7a45d 100644
> --- a/drivers/gpu/drm/armada/armada_drv.c
> +++ b/drivers/gpu/drm/armada/armada_drv.c
> @@ -184,10 +184,12 @@ static void armada_add_endpoints(struct device *dev,
>  
>   for_each_endpoint_of_node(dev_node, ep) {
>   remote = of_graph_get_remote_port_parent(ep);
> - if (remote &&am

Re: [Freedreno] [PATCH 4/5] drm/komeda: Fix IS_ERR() vs NULL check in komeda_component_get_avail_scaler()

2022-11-10 Thread Liviu Dudau 
On Thu, Nov 10, 2022 at 05:44:44PM +0800, Gaosheng Cui wrote:
> The komeda_pipeline_get_state() returns an ERR_PTR() on failure, we should
> use IS_ERR() to check the return value.
> 
> Fixes: 502932a03fce ("drm/komeda: Add the initial scaler support for CORE")
> Signed-off-by: Gaosheng Cui 

Acked-by: Liviu Dudau 

Thanks for the fix!

Best regards,
Liviu

> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
> b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index 3276a3e82c62..e9c92439398d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -259,7 +259,7 @@ komeda_component_get_avail_scaler(struct komeda_component 
> *c,
>   u32 avail_scalers;
>  
>   pipe_st = komeda_pipeline_get_state(c->pipeline, state);
> - if (!pipe_st)
> + if (IS_ERR(pipe_st))
>   return NULL;
>  
>   avail_scalers = (pipe_st->active_comps & KOMEDA_PIPELINE_SCALERS) ^
> -- 
> 2.25.1
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

Re: [Freedreno] [PATCH v2 04/17] drm/msm/dpu: add writeback blocks to the sm8250 DPU catalog

2022-04-21 Thread Liviu Dudau 
On Tue, Apr 19, 2022 at 06:45:56PM -0700, Abhinav Kumar wrote:
> Add writeback blocks to the sm8250 DPU hardware catalog. Other
> chipsets support writeback too but add it to sm8250 to prototype
> the feature so that it can be easily extended to other chipsets.
> 
> changes in v2:
>   - none
> 
> Signed-off-by: Abhinav Kumar 
> Reviewed-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 74 
> +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 66 ++-
>  2 files changed, 138 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index b0a0ef7..bcb5273 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
> +/* Copyright (c) 2022. Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.

Hi Abhinav,

Nit: Order should be historical (i.e. QIC copyright comes last). Comment 
applies to
all other copyright years additions.

Best regards,
Liviu

>   */
>  
>  #define pr_fmt(fmt)  "[drm:%s:%d] " fmt, __func__, __LINE__
> @@ -120,6 +121,16 @@
> BIT(MDP_AD4_0_INTR) | \
> BIT(MDP_AD4_1_INTR))
>  
> +#define WB_SM8250_MASK (BIT(DPU_WB_LINE_MODE) | \
> +  BIT(DPU_WB_UBWC) | \
> +  BIT(DPU_WB_YUV_CONFIG) | \
> +  BIT(DPU_WB_PIPE_ALPHA) | \
> +  BIT(DPU_WB_XY_ROI_OFFSET) | \
> +  BIT(DPU_WB_QOS) | \
> +  BIT(DPU_WB_QOS_8LVL) | \
> +  BIT(DPU_WB_CDP) | \
> +  BIT(DPU_WB_INPUT_CTRL))
> +
>  #define DEFAULT_PIXEL_RAM_SIZE   (50 * 1024)
>  #define DEFAULT_DPU_LINE_WIDTH   2048
>  #define DEFAULT_DPU_OUTPUT_LINE_WIDTH2560
> @@ -211,6 +222,40 @@ static const u32 rotation_v2_formats[] = {
>   /* TODO add formats after validation */
>  };
>  
> +static const uint32_t wb2_formats[] = {
> + DRM_FORMAT_RGB565,
> + DRM_FORMAT_BGR565,
> + DRM_FORMAT_RGB888,
> + DRM_FORMAT_ARGB,
> + DRM_FORMAT_RGBA,
> + DRM_FORMAT_ABGR,
> + DRM_FORMAT_XRGB,
> + DRM_FORMAT_RGBX,
> + DRM_FORMAT_XBGR,
> + DRM_FORMAT_ARGB1555,
> + DRM_FORMAT_RGBA5551,
> + DRM_FORMAT_XRGB1555,
> + DRM_FORMAT_RGBX5551,
> + DRM_FORMAT_ARGB,
> + DRM_FORMAT_RGBA,
> + DRM_FORMAT_RGBX,
> + DRM_FORMAT_XRGB,
> + DRM_FORMAT_BGR565,
> + DRM_FORMAT_BGR888,
> + DRM_FORMAT_ABGR,
> + DRM_FORMAT_BGRA,
> + DRM_FORMAT_BGRX,
> + DRM_FORMAT_XBGR,
> + DRM_FORMAT_ABGR1555,
> + DRM_FORMAT_BGRA5551,
> + DRM_FORMAT_XBGR1555,
> + DRM_FORMAT_BGRX5551,
> + DRM_FORMAT_ABGR,
> + DRM_FORMAT_BGRA,
> + DRM_FORMAT_BGRX,
> + DRM_FORMAT_XBGR,
> +};
> +
>  /*
>   * DPU sub blocks config
>   */
> @@ -448,6 +493,8 @@ static const struct dpu_mdp_cfg sm8250_mdp[] = {
>   .reg_off = 0x2C4, .bit_off = 8},
>   .clk_ctrls[DPU_CLK_CTRL_REG_DMA] = {
>   .reg_off = 0x2BC, .bit_off = 20},
> + .clk_ctrls[DPU_CLK_CTRL_WB2] = {
> + .reg_off = 0x3B8, .bit_off = 24},
>   },
>  };
>  
> @@ -1235,6 +1282,29 @@ static const struct dpu_intf_cfg qcm2290_intf[] = {
>  };
>  
>  /*
> + * Writeback blocks config
> + */
> +#define WB_BLK(_name, _id, _base, _features, _clk_ctrl, \
> + __xin_id, vbif_id, _reg, _wb_done_bit) \
> + { \
> + .name = _name, .id = _id, \
> + .base = _base, .len = 0x2c8, \
> + .features = _features, \
> + .format_list = wb2_formats, \
> + .num_formats = ARRAY_SIZE(wb2_formats), \
> + .clk_ctrl = _clk_ctrl, \
> + .xin_id = __xin_id, \
> + .vbif_idx = vbif_id, \
> + .maxlinewidth = DEFAULT_DPU_LINE_WIDTH, \
> + .intr_wb_done = DPU_IRQ_IDX(_reg, _wb_done_bit) \
> + }
> +
> +static const struct dpu_wb_cfg sm8250_wb[] = {
> + WB_BLK("wb_2", WB_2, 0x65000, WB_SM8250_MASK, DPU_CLK_CTRL_WB2, 6,
> + VBIF_RT, MDP_SSPP_TOP0_INTR, 4),
> +};
> +
> +/*
>   * VBIF sub blocks config
>   */
>  /* VBIF QOS remap */
> @@ -1832,6 +1902,8 @@ static void sm8250_cfg_init(struct dpu_mdss_cfg 
> *dpu_cfg)
>   .intf = sm8150_intf,
>

Re: [Freedreno] [PATCH v7 3/4] drm: allow real encoder to be passed for drm_writeback_connector

2022-04-21 Thread Liviu Dudau 
Hi Abhinav,

Sorry for the delay in reviewing this, Easter break happened in between.

On Fri, Apr 08, 2022 at 05:53:54PM -0700, Abhinav Kumar wrote:
> For some vendor driver implementations, display hardware can
> be shared between the encoder used for writeback and the physical
> display.
> 
> In addition resources such as clocks and interrupts can
> also be shared between writeback and the real encoder.
> 
> To accommodate such vendor drivers and hardware, allow
> real encoder to be passed for drm_writeback_connector.
> 
> For existing clients, drm_writeback_connector_init() will use
> an internal_encoder under the hood and hence no changes will
> be needed.
> 
> changes in v7:
>   - move this change before the vc4 change in the series
> to minimize the changes to vendor drivers in drm core
> changes
> 
> Signed-off-by: Abhinav Kumar 
> ---
>  drivers/gpu/drm/drm_writeback.c | 18 --
>  drivers/gpu/drm/vc4/vc4_txp.c   |  4 ++--
>  include/drm/drm_writeback.h | 22 --
>  3 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 92658ad..0538674 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -180,21 +180,21 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  {
>   int ret = 0;
>  
> - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> + drm_encoder_helper_add(_connector->internal_encoder, 
> enc_helper_funcs);
>  
> - wb_connector->encoder.possible_crtcs = possible_crtcs;
> + wb_connector->internal_encoder.possible_crtcs = possible_crtcs;
>  
> - ret = drm_encoder_init(dev, _connector->encoder,
> + ret = drm_encoder_init(dev, _connector->internal_encoder,
>  _writeback_encoder_funcs,
>  DRM_MODE_ENCODER_VIRTUAL, NULL);
>   if (ret)
>   return ret;
>  
> - ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, 
> _connector->encoder,
> - con_funcs, formats, n_formats);
> + ret = drm_writeback_connector_init_with_encoder(dev, wb_connector,
> + _connector->internal_encoder, con_funcs, formats, 
> n_formats);
>  
>   if (ret)
> - drm_encoder_cleanup(_connector->encoder);
> + drm_encoder_cleanup(_connector->internal_encoder);
>  
>   return ret;
>  }
> @@ -239,6 +239,12 @@ int drm_writeback_connector_init_with_encoder(struct 
> drm_device *dev,
>   struct drm_mode_config *config = >mode_config;
>   int ret = create_writeback_properties(dev);
>  
> + /*
> +  * Assign the encoder passed to this API to the wb_connector's encoder.
> +  * For drm_writeback_connector_init(), this shall be the 
> internal_encoder
> +  */
> + wb_connector->encoder = enc;
> +
>   if (ret != 0)
>   return ret;
>  
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 3447eb6..7e063a9 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -159,7 +159,7 @@ struct vc4_txp {
>  
>  static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
>  {
> - return container_of(encoder, struct vc4_txp, connector.encoder);
> + return container_of(encoder, struct vc4_txp, 
> connector.internal_encoder);

This doesn't feel right. First, vc4_txp should not be aware of the existance of
internal_encoder, that is only for the use of drm_writeback_connector when 
someone
doesn't pass on an encoder. Second, how do we know someone didn't actually use 
the
'encoder' pointer from drm_writeback_connector? I think we should be consistent 
and
only use encoder member.

Best regards,
Liviu

>  }
>  
>  static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector 
> *conn)
> @@ -507,7 +507,7 @@ static int vc4_txp_bind(struct device *dev, struct device 
> *master, void *data)
>   if (ret)
>   return ret;
>  
> - encoder = >connector.encoder;
> + encoder = txp->connector.encoder;
>   encoder->possible_crtcs = drm_crtc_mask(crtc);
>  
>   ret = devm_request_irq(dev, irq, vc4_txp_interrupt, 0,
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index bb306fa..3fbae9d 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -25,13 +25,31 @@ struct drm_writeback_connector {
>   struct drm_connector base;
>  
>   /**
> -  * @encoder: Internal encoder used by the connector to fulfill
> +  * @encoder: handle to drm_encoder used by the connector to fulfill
>* the DRM framework requirements. The users of the
>* @drm_writeback_connector control the behaviour of the @encoder
>* by passing the @enc_funcs parameter to drm_writeback_connector_init()
>* function.
> +  *
> +  * For some vendor drivers, the

Re: [Freedreno] [PATCH v6 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-04-06 Thread Liviu Dudau 
Hi Abhinav,

On Thu, Mar 31, 2022 at 05:12:11PM -0700, Abhinav Kumar wrote:
> For vendors drivers which pass an already allocated and
> initialized encoder especially for cases where the encoder
> hardware is shared OR the writeback encoder shares the resources
> with the rest of the display pipeline introduce a new API,
> drm_writeback_connector_init_with_encoder() which expects
> an initialized encoder as a parameter and only sets up the
> writeback connector.
> 
> changes in v6:
>   - remove drm_writeback_connector_setup() and instead
> directly call drm_writeback_connector_init_with_encoder()
>   - fix a drm_writeback_connector typo and function doc which
> incorrectly shows that the function accepts enc_helper_funcs
>   - pass encoder as a parameter explicitly to the new API
> for better readability

Side comment: if you plan to have the log of changes in the commit message then 
I
would keep a list of all the changes. Otherwise, putting the log after the -- 
line
below would still convey the information but will also ensure that when merged 
it
will not show up in the commit message.

> 
> Signed-off-by: Abhinav Kumar 
> ---
>  drivers/gpu/drm/drm_writeback.c | 72 
> +
>  include/drm/drm_writeback.h |  6 
>  2 files changed, 64 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dc2ef12..797223c 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -177,6 +177,62 @@ int drm_writeback_connector_init(struct drm_device *dev,
>const struct drm_encoder_helper_funcs 
> *enc_helper_funcs,
>const u32 *formats, int n_formats, uint32_t 
> possible_crtcs)
>  {
> + int ret = 0;
> +
> + drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> +
> + wb_connector->encoder.possible_crtcs = possible_crtcs;
> +
> + ret = drm_encoder_init(dev, _connector->encoder,
> +_writeback_encoder_funcs,
> +DRM_MODE_ENCODER_VIRTUAL, NULL);
> + if (ret)
> + return ret;
> +
> + ret = drm_writeback_connector_init_with_encoder(dev, wb_connector, 
> _connector->encoder,
> + con_funcs, formats, n_formats);
> +
> + if (ret)
> + drm_encoder_cleanup(_connector->encoder);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_writeback_connector_init);
> +
> +/**
> + * drm_writeback_connector_init_with_encoder - Initialize a writeback 
> connector and its properties
> + * using the encoder which already assigned and initialized
> + *
> + * @dev: DRM device
> + * @wb_connector: Writeback connector to initialize
> + * @enc: handle to the already initialized drm encoder
> + * @con_funcs: Connector funcs vtable
> + * @formats: Array of supported pixel formats for the writeback engine
> + * @n_formats: Length of the formats array
> + *
> + * This function creates the writeback-connector-specific properties if they
> + * have not been already created, initializes the connector as
> + * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
> + * values.
> + *
> + * This function assumes that the drm_writeback_connector's encoder has 
> already been
> + * created and initialized before invoking this function.
> + *
> + * In addition, this function also assumes that callers of this API will 
> manage
> + * assigning the encoder helper functions, possible_crtcs and any other 
> encoder
> + * specific operation which is otherwise handled by 
> drm_writeback_connector_init().

I would stop after "... specific operation".

Otherwise, looks good to me.

Reviewed-by: Liviu Dudau 

Best regards,
Liviu


> + *
> + * Drivers should always use this function instead of drm_connector_init() to
> + * set up writeback connectors if they want to manage themselves the 
> lifetime of the
> + * associated encoder.
> + *
> + * Returns: 0 on success, or a negative error code
> + */
> +int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> + struct drm_writeback_connector *wb_connector, struct 
> drm_encoder *enc,
> + const struct drm_connector_funcs *con_funcs, const u32 *formats,
> + int n_formats)
> +{
>   struct drm_property_blob *blob;
>   struct drm_connector *connector = _connector->base;
>   struct drm_mode_config *config = >mode_config;
> @@ -190,15 +246,6 @@ int drm_writeback_connector_init(struct drm_device *dev,
>   if (IS_ERR(blob))
>   ret

Re: [Freedreno] [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-31 Thread Liviu Dudau 
On Fri, Mar 25, 2022 at 09:31:35AM -0700, Abhinav Kumar wrote:
> Hi Liviu

Hi Abhinav,

Sorry for the delay, got busy with other things at the beginning of the week.

> 
> On 3/25/2022 3:19 AM, Liviu Dudau wrote:
> > On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:
> > > Hi Liviu
> > 
> > Hello,
> > 
> > > 
> > > Thanks for the response.
> > > 
> > > On 3/24/2022 3:12 AM, Liviu Dudau wrote:
> > > > On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
> > > > > Hi Liviu
> > > > 
> > > > Hello,
> > > > 
> > > > > 
> > > > > Thanks for the review.
> > > > > 
> > > > > On 3/23/2022 9:46 AM, Liviu Dudau wrote:
> > > > > > On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
> > > > > > > For vendors drivers which pass an already allocated and
> > > > > > > initialized encoder especially for cases where the encoder
> > > > > > > hardware is shared OR the writeback encoder shares the resources
> > > > > > > with the rest of the display pipeline introduce a new API,
> > > > > > > drm_writeback_connector_init_with_encoder() which expects
> > > > > > > an initialized encoder as a parameter and only sets up the
> > > > > > > writeback connector.
> > > > > > > 
> > > > > > > changes in v5:
> > > > > > >   - reorder this change to come before in the series
> > > > > > > to avoid incorrect functionality in subsequent changes
> > > > > > >   - continue using struct drm_encoder instead of
> > > > > > > struct drm_encoder * and switch it in next change
> > > > > > > 
> > > > > > > Signed-off-by: Abhinav Kumar 
> > > > > > 
> > > > > > Hi Abhinav,
> > > > > > 
> > > > > > > ---
> > > > > > > drivers/gpu/drm/drm_writeback.c | 143 
> > > > > > > 
> > > > > > > include/drm/drm_writeback.h |   5 ++
> > > > > > > 2 files changed, 106 insertions(+), 42 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c 
> > > > > > > b/drivers/gpu/drm/drm_writeback.c
> > > > > > > index dc2ef12..abe78b9 100644
> > > > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > > > @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs 
> > > > > > > drm_writeback_encoder_funcs = {
> > > > > > >   .destroy = drm_encoder_cleanup,
> > > > > > > };
> > > > > > > -/**
> > > > > > > - * drm_writeback_connector_init - Initialize a writeback 
> > > > > > > connector and its properties
> > > > > > > - * @dev: DRM device
> > > > > > > - * @wb_connector: Writeback connector to initialize
> > > > > > > - * @con_funcs: Connector funcs vtable
> > > > > > > - * @enc_helper_funcs: Encoder helper funcs vtable to be used by 
> > > > > > > the internal encoder
> > > > > > > - * @formats: Array of supported pixel formats for the writeback 
> > > > > > > engine
> > > > > > > - * @n_formats: Length of the formats array
> > > > > > > - * @possible_crtcs: possible crtcs for the internal writeback 
> > > > > > > encoder
> > > > > > > - *
> > > > > > > - * This function creates the writeback-connector-specific 
> > > > > > > properties if they
> > > > > > > - * have not been already created, initializes the connector as
> > > > > > > - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes 
> > > > > > > the property
> > > > > > > - * values. It will also create an internal encoder associated 
> > > > > > > with the
> > > > > > > - * drm_writeback_connector and set it to use the 
> > > > > > > @enc_helper_funcs vtable for
> > > > > > > - * the encoder helper.
> > > > > >

Re: [Freedreno] [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-25 Thread Liviu Dudau 
On Thu, Mar 24, 2022 at 09:36:50AM -0700, Abhinav Kumar wrote:
> Hi Liviu

Hello,

> 
> Thanks for the response.
> 
> On 3/24/2022 3:12 AM, Liviu Dudau wrote:
> > On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
> > > Hi Liviu
> > 
> > Hello,
> > 
> > > 
> > > Thanks for the review.
> > > 
> > > On 3/23/2022 9:46 AM, Liviu Dudau wrote:
> > > > On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
> > > > > For vendors drivers which pass an already allocated and
> > > > > initialized encoder especially for cases where the encoder
> > > > > hardware is shared OR the writeback encoder shares the resources
> > > > > with the rest of the display pipeline introduce a new API,
> > > > > drm_writeback_connector_init_with_encoder() which expects
> > > > > an initialized encoder as a parameter and only sets up the
> > > > > writeback connector.
> > > > > 
> > > > > changes in v5:
> > > > >   - reorder this change to come before in the series
> > > > > to avoid incorrect functionality in subsequent changes
> > > > >   - continue using struct drm_encoder instead of
> > > > > struct drm_encoder * and switch it in next change
> > > > > 
> > > > > Signed-off-by: Abhinav Kumar 
> > > > 
> > > > Hi Abhinav,
> > > > 
> > > > > ---
> > > > >drivers/gpu/drm/drm_writeback.c | 143 
> > > > > 
> > > > >include/drm/drm_writeback.h |   5 ++
> > > > >2 files changed, 106 insertions(+), 42 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_writeback.c 
> > > > > b/drivers/gpu/drm/drm_writeback.c
> > > > > index dc2ef12..abe78b9 100644
> > > > > --- a/drivers/gpu/drm/drm_writeback.c
> > > > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > > > @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs 
> > > > > drm_writeback_encoder_funcs = {
> > > > >   .destroy = drm_encoder_cleanup,
> > > > >};
> > > > > -/**
> > > > > - * drm_writeback_connector_init - Initialize a writeback connector 
> > > > > and its properties
> > > > > - * @dev: DRM device
> > > > > - * @wb_connector: Writeback connector to initialize
> > > > > - * @con_funcs: Connector funcs vtable
> > > > > - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the 
> > > > > internal encoder
> > > > > - * @formats: Array of supported pixel formats for the writeback 
> > > > > engine
> > > > > - * @n_formats: Length of the formats array
> > > > > - * @possible_crtcs: possible crtcs for the internal writeback encoder
> > > > > - *
> > > > > - * This function creates the writeback-connector-specific properties 
> > > > > if they
> > > > > - * have not been already created, initializes the connector as
> > > > > - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the 
> > > > > property
> > > > > - * values. It will also create an internal encoder associated with 
> > > > > the
> > > > > - * drm_writeback_connector and set it to use the @enc_helper_funcs 
> > > > > vtable for
> > > > > - * the encoder helper.
> > > > > - *
> > > > > - * Drivers should always use this function instead of 
> > > > > drm_connector_init() to
> > > > > - * set up writeback connectors.
> > > > > - *
> > > > > - * Returns: 0 on success, or a negative error code
> > > > > - */
> > > > > -int drm_writeback_connector_init(struct drm_device *dev,
> > > > > -  struct drm_writeback_connector 
> > > > > *wb_connector,
> > > > > -  const struct drm_connector_funcs 
> > > > > *con_funcs,
> > > > > -  const struct drm_encoder_helper_funcs 
> > > > > *enc_helper_funcs,
> > > > > -  const u32 *formats, int n_formats, 
> > > > > uint32_t possible_crtcs)
> > > > > +static int drm_writeback_connecto

Re: [Freedreno] [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-24 Thread Liviu Dudau 
On Wed, Mar 23, 2022 at 11:28:56AM -0700, Abhinav Kumar wrote:
> Hi Liviu

Hello,

> 
> Thanks for the review.
> 
> On 3/23/2022 9:46 AM, Liviu Dudau wrote:
> > On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
> > > For vendors drivers which pass an already allocated and
> > > initialized encoder especially for cases where the encoder
> > > hardware is shared OR the writeback encoder shares the resources
> > > with the rest of the display pipeline introduce a new API,
> > > drm_writeback_connector_init_with_encoder() which expects
> > > an initialized encoder as a parameter and only sets up the
> > > writeback connector.
> > > 
> > > changes in v5:
> > >   - reorder this change to come before in the series
> > > to avoid incorrect functionality in subsequent changes
> > >   - continue using struct drm_encoder instead of
> > > struct drm_encoder * and switch it in next change
> > > 
> > > Signed-off-by: Abhinav Kumar 
> > 
> > Hi Abhinav,
> > 
> > > ---
> > >   drivers/gpu/drm/drm_writeback.c | 143 
> > > 
> > >   include/drm/drm_writeback.h |   5 ++
> > >   2 files changed, 106 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_writeback.c 
> > > b/drivers/gpu/drm/drm_writeback.c
> > > index dc2ef12..abe78b9 100644
> > > --- a/drivers/gpu/drm/drm_writeback.c
> > > +++ b/drivers/gpu/drm/drm_writeback.c
> > > @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs 
> > > drm_writeback_encoder_funcs = {
> > >   .destroy = drm_encoder_cleanup,
> > >   };
> > > -/**
> > > - * drm_writeback_connector_init - Initialize a writeback connector and 
> > > its properties
> > > - * @dev: DRM device
> > > - * @wb_connector: Writeback connector to initialize
> > > - * @con_funcs: Connector funcs vtable
> > > - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the 
> > > internal encoder
> > > - * @formats: Array of supported pixel formats for the writeback engine
> > > - * @n_formats: Length of the formats array
> > > - * @possible_crtcs: possible crtcs for the internal writeback encoder
> > > - *
> > > - * This function creates the writeback-connector-specific properties if 
> > > they
> > > - * have not been already created, initializes the connector as
> > > - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the 
> > > property
> > > - * values. It will also create an internal encoder associated with the
> > > - * drm_writeback_connector and set it to use the @enc_helper_funcs 
> > > vtable for
> > > - * the encoder helper.
> > > - *
> > > - * Drivers should always use this function instead of 
> > > drm_connector_init() to
> > > - * set up writeback connectors.
> > > - *
> > > - * Returns: 0 on success, or a negative error code
> > > - */
> > > -int drm_writeback_connector_init(struct drm_device *dev,
> > > -  struct drm_writeback_connector *wb_connector,
> > > -  const struct drm_connector_funcs *con_funcs,
> > > -  const struct drm_encoder_helper_funcs 
> > > *enc_helper_funcs,
> > > -  const u32 *formats, int n_formats, uint32_t 
> > > possible_crtcs)
> > > +static int drm_writeback_connector_setup(struct drm_device *dev,
> > > + struct drm_writeback_connector *wb_connector,
> > > + const struct drm_connector_funcs *con_funcs, const u32 *formats,
> > > + int n_formats)
> > >   {
> > >   struct drm_property_blob *blob;
> > > - struct drm_connector *connector = _connector->base;
> > >   struct drm_mode_config *config = >mode_config;
> > > + struct drm_connector *connector = _connector->base;
> > > +
> > 
> > Point of this reordering the statements is...?
> This diff can be avoided. There was no reason to reorder this. I will remove
> this re-order.
> > 
> > >   int ret = create_writeback_properties(dev);
> > >   if (ret != 0)
> > > @@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct drm_device 
> > > *dev,
> > >   blob = drm_property_create_blob(dev, n_formats * 
> > > sizeof(*formats),
> > >

Re: [Freedreno] [PATCH v5 2/4] drm: introduce drm_writeback_connector_init_with_encoder() API

2022-03-23 Thread Liviu Dudau 
On Mon, Mar 21, 2022 at 04:56:43PM -0700, Abhinav Kumar wrote:
> For vendors drivers which pass an already allocated and
> initialized encoder especially for cases where the encoder
> hardware is shared OR the writeback encoder shares the resources
> with the rest of the display pipeline introduce a new API,
> drm_writeback_connector_init_with_encoder() which expects
> an initialized encoder as a parameter and only sets up the
> writeback connector.
> 
> changes in v5:
>   - reorder this change to come before in the series
> to avoid incorrect functionality in subsequent changes
>   - continue using struct drm_encoder instead of
> struct drm_encoder * and switch it in next change
> 
> Signed-off-by: Abhinav Kumar 

Hi Abhinav,

> ---
>  drivers/gpu/drm/drm_writeback.c | 143 
> 
>  include/drm/drm_writeback.h |   5 ++
>  2 files changed, 106 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dc2ef12..abe78b9 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -149,37 +149,15 @@ static const struct drm_encoder_funcs 
> drm_writeback_encoder_funcs = {
>   .destroy = drm_encoder_cleanup,
>  };
>  
> -/**
> - * drm_writeback_connector_init - Initialize a writeback connector and its 
> properties
> - * @dev: DRM device
> - * @wb_connector: Writeback connector to initialize
> - * @con_funcs: Connector funcs vtable
> - * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal 
> encoder
> - * @formats: Array of supported pixel formats for the writeback engine
> - * @n_formats: Length of the formats array
> - * @possible_crtcs: possible crtcs for the internal writeback encoder
> - *
> - * This function creates the writeback-connector-specific properties if they
> - * have not been already created, initializes the connector as
> - * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
> - * values. It will also create an internal encoder associated with the
> - * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
> - * the encoder helper.
> - *
> - * Drivers should always use this function instead of drm_connector_init() to
> - * set up writeback connectors.
> - *
> - * Returns: 0 on success, or a negative error code
> - */
> -int drm_writeback_connector_init(struct drm_device *dev,
> -  struct drm_writeback_connector *wb_connector,
> -  const struct drm_connector_funcs *con_funcs,
> -  const struct drm_encoder_helper_funcs 
> *enc_helper_funcs,
> -  const u32 *formats, int n_formats, uint32_t 
> possible_crtcs)
> +static int drm_writeback_connector_setup(struct drm_device *dev,
> + struct drm_writeback_connector *wb_connector,
> + const struct drm_connector_funcs *con_funcs, const u32 *formats,
> + int n_formats)
>  {
>   struct drm_property_blob *blob;
> - struct drm_connector *connector = _connector->base;
>   struct drm_mode_config *config = >mode_config;
> + struct drm_connector *connector = _connector->base;
> +

Point of this reordering the statements is...?

>   int ret = create_writeback_properties(dev);
>  
>   if (ret != 0)
> @@ -187,18 +165,10 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  
>   blob = drm_property_create_blob(dev, n_formats * sizeof(*formats),
>   formats);
> - if (IS_ERR(blob))
> - return PTR_ERR(blob);
> -
> - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> -
> - wb_connector->encoder.possible_crtcs = possible_crtcs;
> -
> - ret = drm_encoder_init(dev, _connector->encoder,
> -_writeback_encoder_funcs,
> -DRM_MODE_ENCODER_VIRTUAL, NULL);
> - if (ret)
> - goto fail;
> + if (IS_ERR(blob)) {
> + ret = PTR_ERR(blob);
> + return ret;
> + }

I don't see why you have changed the earlier code to store the result of 
PTR_ERR into
ret other than to help your debugging. I suggest that you keep the existing 
code that
returns PTR_ERR(blob) directly and you will have a nicer diff stat as well.

>  
>   connector->interlace_allowed = 0;
>  
> @@ -237,13 +207,102 @@ int drm_writeback_connector_init(struct drm_device 
> *dev,
>  attach_fail:
>   drm_connector_cleanup(connector);
>  connector_fail:
> - drm_encoder_cleanup(_connector->encoder);
> -fail:
>   drm_property_blob_put(blob);
>   return ret;
>  }
> +
> +/**
> + * drm_writeback_connector_init - Initialize a writeback connector and its 
> properties
> + * @dev: DRM device
> + * @wb_connector: Writeback connector to initialize
> + * @con_funcs: Connector funcs vtable
> + * @enc_helper_funcs: Encoder helper funcs

Re: [Freedreno] [PATCH v3 1/3] drm: allow real encoder to be passed for drm_writeback_connector

2022-03-21 Thread Liviu Dudau 
On Thu, Mar 17, 2022 at 10:26:38AM -0700, Abhinav Kumar wrote:
> Hi Laurent
> 
> Thanks for the review.
> 
> On 3/17/2022 1:51 AM, Laurent Pinchart wrote:
> > Hi Abhinav,
> > 
> > Thank you for the patch.
> > 
> > On Wed, Mar 16, 2022 at 11:48:16AM -0700, Abhinav Kumar wrote:
> > > For some vendor driver implementations, display hardware can
> > > be shared between the encoder used for writeback and the physical
> > > display.
> > > 
> > > In addition resources such as clocks and interrupts can
> > > also be shared between writeback and the real encoder.
> > > 
> > > To accommodate such vendor drivers and hardware, allow
> > > real encoder to be passed for drm_writeback_connector using a new
> > > drm_writeback_connector_init_with_encoder() API.
> > 
> > The commit message doesn't match the commit.
> Sorry, while splitting the change , I missed this part of the commit text.
> Will fix it up.
> > 
> > > In addition, to preserve the same call flows for the existing
> > > users of drm_writeback_connector_init(), also allow passing
> > > possible_crtcs as a parameter so that encoder can be initialized
> > > with it.
> > > 
> > > changes in v3:
> > >   - allow passing possible_crtcs for existing users of
> > > drm_writeback_connector_init()
> > >   - squash the vendor changes into the same commit so
> > > that each patch in the series can compile individually
> > > 
> > > Co-developed-by: Kandpal Suraj 
> > > Signed-off-by: Abhinav Kumar 
> > > ---
> > >   .../drm/arm/display/komeda/komeda_wb_connector.c   |   3 +-
> > >   drivers/gpu/drm/arm/malidp_mw.c|   5 +-
> > >   drivers/gpu/drm/drm_writeback.c| 103 
> > > +
> > >   drivers/gpu/drm/rcar-du/rcar_du_writeback.c|   5 +-
> > >   drivers/gpu/drm/vc4/vc4_txp.c  |  19 ++--
> > >   drivers/gpu/drm/vkms/vkms_writeback.c  |   3 +-
> > >   include/drm/drm_writeback.h|  22 -
> > >   7 files changed, 103 insertions(+), 57 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c 
> > > b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > index e465cc4..40774e6 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
> > > @@ -155,7 +155,6 @@ static int komeda_wb_connector_add(struct 
> > > komeda_kms_dev *kms,
> > >   kwb_conn->wb_layer = kcrtc->master->wb_layer;
> > >   wb_conn = _conn->base;
> > > - wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(>base));
> > >   formats = komeda_get_layer_fourcc_list(>fmt_tbl,
> > >  
> > > kwb_conn->wb_layer->layer_type,
> > > @@ -164,7 +163,7 @@ static int komeda_wb_connector_add(struct 
> > > komeda_kms_dev *kms,
> > >   err = drm_writeback_connector_init(>base, wb_conn,
> > >  _wb_connector_funcs,
> > >  
> > > _wb_encoder_helper_funcs,
> > > -formats, n_formats);
> > > +formats, n_formats, 
> > > BIT(drm_crtc_index(>base)));
> > >   komeda_put_fourcc_list(formats);
> > >   if (err) {
> > >   kfree(kwb_conn);
> > > diff --git a/drivers/gpu/drm/arm/malidp_mw.c 
> > > b/drivers/gpu/drm/arm/malidp_mw.c
> > > index f5847a7..b882066 100644
> > > --- a/drivers/gpu/drm/arm/malidp_mw.c
> > > +++ b/drivers/gpu/drm/arm/malidp_mw.c
> > > @@ -208,11 +208,12 @@ int malidp_mw_connector_init(struct drm_device *drm)
> > >   struct malidp_drm *malidp = drm->dev_private;
> > >   u32 *formats;
> > >   int ret, n_formats;
> > > + uint32_t possible_crtcs;
> > >   if (!malidp->dev->hw->enable_memwrite)
> > >   return 0;
> > > - malidp->mw_connector.encoder.possible_crtcs = 1 << 
> > > drm_crtc_index(>crtc);
> > > + possible_crtcs = 1 << drm_crtc_index(>crtc);
> > >   drm_connector_helper_add(>mw_connector.base,
> > >_mw_connector_helper_funcs);
> > > @@ -223,7 +224,7 @@ int malidp_mw_connector_init(struct drm_device *drm)
> > >   ret = drm_writeback_connector_init(drm, >mw_connector,
> > >  _mw_connector_funcs,
> > >  
> > > _mw_encoder_helper_funcs,
> > > -formats, n_formats);
> > > +formats, n_formats, possible_crtcs);
> > 
> > Do you need the local variable ?
> 
> Yes, we can dtop this. I just used this instead of "1 <<
> drm_crtc_index(>crtc)" to simplify it.
> No strong preference.
> 
> > 
> > >   kfree(formats);
> > >   if (ret)
> > >   return ret;
> > > diff --git a/drivers/gpu/drm/drm_writeback.c 
> > >

Re: [Freedreno] [PATCH v4 2/4] drm: allow real encoder to be passed for drm_writeback_connector

2022-03-21 Thread Liviu Dudau 
On Thu, Mar 17, 2022 at 06:45:34PM -0700, Abhinav Kumar wrote:
> For some vendor driver implementations, display hardware can
> be shared between the encoder used for writeback and the physical
> display.
> 
> In addition resources such as clocks and interrupts can
> also be shared between writeback and the real encoder.
> 
> To accommodate such vendor drivers and hardware, allow
> real encoder to be passed for drm_writeback_connector.
> 
> changes in v4:
>   - split the possible_crtcs change and the parts which should
> belong to the addition of new API to the next change
> 
> Co-developed-by: Kandpal Suraj 
> Signed-off-by: Abhinav Kumar 
> ---
>  drivers/gpu/drm/drm_writeback.c | 12 +++-
>  drivers/gpu/drm/vc4/vc4_txp.c   | 14 ++
>  include/drm/drm_writeback.h | 18 --
>  3 files changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dc2ef12..a4c17d6 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -190,11 +190,13 @@ int drm_writeback_connector_init(struct drm_device *dev,
>   if (IS_ERR(blob))
>   return PTR_ERR(blob);
>  
> - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
>  
> - wb_connector->encoder.possible_crtcs = possible_crtcs;
> + wb_connector->encoder = _connector->internal_encoder;

You need to check here that the wb_connector doesn't have already an attached 
encoder
before you overwrite the pointer with the internal encoder.

>  
> - ret = drm_encoder_init(dev, _connector->encoder,
> + wb_connector->encoder->possible_crtcs = possible_crtcs;
> +
> + ret = drm_encoder_init(dev, wb_connector->encoder,
>  _writeback_encoder_funcs,
>  DRM_MODE_ENCODER_VIRTUAL, NULL);

Here you have initialised the encoder pointed at by wb_connector->encoder, 
which is
always wb_connector->internal_encoder with your code.

>   if (ret)
> @@ -208,7 +210,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>   goto connector_fail;
>  
>   ret = drm_connector_attach_encoder(connector,
> - _connector->encoder);
> + wb_connector->encoder);
>   if (ret)
>   goto attach_fail;
>  
> @@ -237,7 +239,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  attach_fail:
>   drm_connector_cleanup(connector);
>  connector_fail:
> - drm_encoder_cleanup(_connector->encoder);
> + drm_encoder_cleanup(wb_connector->encoder);
>  fail:
>   drm_property_blob_put(blob);
>   return ret;
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 3447eb6..341a9be5 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -151,6 +151,8 @@ struct vc4_txp {
>  
>   struct platform_device *pdev;
>  
> + struct drm_encoder drm_enc;
> +
>   struct drm_writeback_connector connector;
>  
>   void __iomem *regs;
> @@ -159,7 +161,7 @@ struct vc4_txp {
>  
>  static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
>  {
> - return container_of(encoder, struct vc4_txp, connector.encoder);
> + return container_of(encoder, struct vc4_txp, drm_enc);
>  }
>  
>  static inline struct vc4_txp *connector_to_vc4_txp(struct drm_connector 
> *conn)
> @@ -467,6 +469,7 @@ static int vc4_txp_bind(struct device *dev, struct device 
> *master, void *data)
>   struct vc4_txp *txp;
>   struct drm_crtc *crtc;
>   struct drm_encoder *encoder;
> + struct drm_writeback_connector *wb_conn;
>   int ret, irq;
>  
>   irq = platform_get_irq(pdev, 0);
> @@ -492,9 +495,12 @@ static int vc4_txp_bind(struct device *dev, struct 
> device *master, void *data)
>   txp->regset.regs = txp_regs;
>   txp->regset.nregs = ARRAY_SIZE(txp_regs);
>  
> - drm_connector_helper_add(>connector.base,
> + wb_conn = >connector;
> + wb_conn->encoder = >drm_enc;
> +
> + drm_connector_helper_add(_conn->base,
>_txp_connector_helper_funcs);
> - ret = drm_writeback_connector_init(drm, >connector,
> + ret = drm_writeback_connector_init(drm, wb_conn,
>  _txp_connector_funcs,
>  _txp_encoder_helper_funcs,
>  drm_fmts, ARRAY_SIZE(drm_fmts),

This call will never initialise the txp->drm_enc, as per my comments above. 
However
if this was the intent, it's fine, but then you need to add a 
drm_encoder_init() call
here for txp->drm_enc. Otherwise, you need to stop overwriting the pointer in
drm_writeback_connector_init().

> @@ -507,7 +513,7 @@ static int vc4_txp_bind(struct device *dev, struct

Re: [Freedreno] [PATCH v4 1/4] drm: allow passing possible_crtcs to drm_writeback_connector_init()

2022-03-21 Thread Liviu Dudau 
s)
>  {
>   struct drm_property_blob *blob;
>   struct drm_connector *connector = _connector->base;
> @@ -190,6 +191,9 @@ int drm_writeback_connector_init(struct drm_device *dev,
>   return PTR_ERR(blob);
>  
>   drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> +
> + wb_connector->encoder.possible_crtcs = possible_crtcs;
> +
>   ret = drm_encoder_init(dev, _connector->encoder,
>  _writeback_encoder_funcs,
>  DRM_MODE_ENCODER_VIRTUAL, NULL);

For all of the above changes:

Acked-by: Liviu Dudau 

Best regards,
Liviu

> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> index c79d125..fcfb0b3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c
> @@ -200,7 +200,6 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>  {
>   struct drm_writeback_connector *wb_conn = >writeback;
>  
> - wb_conn->encoder.possible_crtcs = 1 << drm_crtc_index(>crtc);
>   drm_connector_helper_add(_conn->base,
>_du_wb_conn_helper_funcs);
>  
> @@ -208,7 +207,8 @@ int rcar_du_writeback_init(struct rcar_du_device *rcdu,
>   _du_wb_conn_funcs,
>   _du_wb_enc_helper_funcs,
>   writeback_formats,
> - ARRAY_SIZE(writeback_formats));
> + ARRAY_SIZE(writeback_formats),
> +(1 << drm_crtc_index(>crtc)));
>  }
>  
>  void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc,
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index 9809ca3..3447eb6 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -497,7 +497,8 @@ static int vc4_txp_bind(struct device *dev, struct device 
> *master, void *data)
>   ret = drm_writeback_connector_init(drm, >connector,
>  _txp_connector_funcs,
>  _txp_encoder_helper_funcs,
> -drm_fmts, ARRAY_SIZE(drm_fmts));
> +drm_fmts, ARRAY_SIZE(drm_fmts),
> +0);
>   if (ret)
>   return ret;
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c 
> b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 8694227..6d01e55 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -140,12 +140,12 @@ int vkms_enable_writeback_connector(struct vkms_device 
> *vkmsdev)
>  {
>   struct drm_writeback_connector *wb = >output.wb_connector;
>  
> - vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
>   drm_connector_helper_add(>base, _wb_conn_helper_funcs);
>  
>   return drm_writeback_connector_init(>drm, wb,
>   _wb_connector_funcs,
>   _wb_encoder_helper_funcs,
>   vkms_wb_formats,
> - ARRAY_SIZE(vkms_wb_formats));
> + ARRAY_SIZE(vkms_wb_formats),
> + 1);
>  }
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 9697d27..db6214f 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -150,7 +150,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>struct drm_writeback_connector *wb_connector,
>const struct drm_connector_funcs *con_funcs,
>const struct drm_encoder_helper_funcs 
> *enc_helper_funcs,
> -  const u32 *formats, int n_formats);
> +  const u32 *formats, int n_formats, uint32_t 
> possible_crtcs);
>  
>  int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>struct drm_framebuffer *fb);
> -- 
> 2.7.4
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

Re: [Freedreno] [PATCH v2 02/14] drm/arm/hdlcd: Convert to Linux IRQ interfaces

2021-08-20 Thread Liviu Dudau 
On Tue, Aug 03, 2021 at 11:06:52AM +0200, Thomas Zimmermann wrote:
> Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's
> IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers
> don't benefit from using it.
> 
> DRM IRQ callbacks are now being called directly or inlined.
> 
> Calls to platform_get_irq() can fail with a negative errno code.
> Abort initialization in this case. The DRM IRQ midlayer does not
> handle this case correctly.
> 
> v2:
>   * name struct drm_device variables 'drm' (Sam)
> 
> Signed-off-by: Thomas Zimmermann 
> Acked-by: Sam Ravnborg 

Sorry for the delayed response due to holidays.

Acked-by: Liviu Dudau 

Best regards,
Liviu

> ---
>  drivers/gpu/drm/arm/hdlcd_drv.c | 174 ++--
>  drivers/gpu/drm/arm/hdlcd_drv.h |   1 +
>  2 files changed, 97 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index 81ae92390736..479c2422a2e0 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -29,7 +29,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -38,6 +37,94 @@
>  #include "hdlcd_drv.h"
>  #include "hdlcd_regs.h"
>  
> +static irqreturn_t hdlcd_irq(int irq, void *arg)
> +{
> + struct drm_device *drm = arg;
> + struct hdlcd_drm_private *hdlcd = drm->dev_private;
> + unsigned long irq_status;
> +
> + irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
> +
> +#ifdef CONFIG_DEBUG_FS
> + if (irq_status & HDLCD_INTERRUPT_UNDERRUN)
> + atomic_inc(>buffer_underrun_count);
> +
> + if (irq_status & HDLCD_INTERRUPT_DMA_END)
> + atomic_inc(>dma_end_count);
> +
> + if (irq_status & HDLCD_INTERRUPT_BUS_ERROR)
> + atomic_inc(>bus_error_count);
> +
> + if (irq_status & HDLCD_INTERRUPT_VSYNC)
> + atomic_inc(>vsync_count);
> +
> +#endif
> + if (irq_status & HDLCD_INTERRUPT_VSYNC)
> + drm_crtc_handle_vblank(>crtc);
> +
> + /* acknowledge interrupt(s) */
> + hdlcd_write(hdlcd, HDLCD_REG_INT_CLEAR, irq_status);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void hdlcd_irq_preinstall(struct drm_device *drm)
> +{
> + struct hdlcd_drm_private *hdlcd = drm->dev_private;
> + /* Ensure interrupts are disabled */
> + hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, 0);
> + hdlcd_write(hdlcd, HDLCD_REG_INT_CLEAR, ~0);
> +}
> +
> +static void hdlcd_irq_postinstall(struct drm_device *drm)
> +{
> +#ifdef CONFIG_DEBUG_FS
> + struct hdlcd_drm_private *hdlcd = drm->dev_private;
> + unsigned long irq_mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
> +
> + /* enable debug interrupts */
> + irq_mask |= HDLCD_DEBUG_INT_MASK;
> +
> + hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
> +#endif
> +}
> +
> +static int hdlcd_irq_install(struct drm_device *drm, int irq)
> +{
> + int ret;
> +
> + if (irq == IRQ_NOTCONNECTED)
> + return -ENOTCONN;
> +
> + hdlcd_irq_preinstall(drm);
> +
> + ret = request_irq(irq, hdlcd_irq, 0, drm->driver->name, drm);
> + if (ret)
> + return ret;
> +
> + hdlcd_irq_postinstall(drm);
> +
> + return 0;
> +}
> +
> +static void hdlcd_irq_uninstall(struct drm_device *drm)
> +{
> + struct hdlcd_drm_private *hdlcd = drm->dev_private;
> + /* disable all the interrupts that we might have enabled */
> + unsigned long irq_mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
> +
> +#ifdef CONFIG_DEBUG_FS
> + /* disable debug interrupts */
> + irq_mask &= ~HDLCD_DEBUG_INT_MASK;
> +#endif
> +
> + /* disable vsync interrupts */
> + irq_mask &= ~HDLCD_INTERRUPT_VSYNC;
> + hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
> +
> + free_irq(hdlcd->irq, drm);
> +}
> +
>  static int hdlcd_load(struct drm_device *drm, unsigned long flags)
>  {
>   struct hdlcd_drm_private *hdlcd = drm->dev_private;
> @@ -90,7 +177,12 @@ static int hdlcd_load(struct drm_device *drm, unsigned 
> long flags)
>   goto setup_fail;
>   }
>  
> - ret = drm_irq_install(drm, platform_get_irq(pdev, 0));
> + ret = platform_get_irq(pdev, 0);
> + if (ret < 0)
> + goto irq_fail;
> + hdlcd->irq = ret;
> +
> + ret = hdlcd_irq_install(drm, hdlcd->irq);
>   if (ret < 0) {
>   DRM_ERROR("failed to install IRQ handler\n");
>

Re: [Freedreno] [PATCH v2] drm: prefix header search paths with $(srctree)/

2019-03-29 Thread Liviu Dudau 
On Fri, Mar 29, 2019 at 08:32:41PM +0900, Masahiro Yamada wrote:
> Currently, the Kbuild core manipulates header search paths in a crazy
> way [1].
> 
> To fix this mess, I want all Makefiles to add explicit $(srctree)/ to
> the search paths in the srctree. Some Makefiles are already written in
> that way, but not all. The goal of this work is to make the notation
> consistent, and finally get rid of the gross hacks.
> 
> Having whitespaces after -I does not matter since commit 48f6e3cf5bc6
> ("kbuild: do not drop -I without parameter").
> 
> [1]: https://patchwork.kernel.org/patch/9632347/
> 
> Signed-off-by: Masahiro Yamada 
> Reviewed-by: Sam Ravnborg 
> ---
> 
> I put all gpu/drm changes into a single patch because
> they are trivial conversion.
> 
> If you are interested in the big picture of this work,
> the full patch set is available at the following URL.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git 
> build-test
> 
> 
> Changes in v2:
>   - fix up the new driver komeda
>   - Add Sam's Reviewed-by
> 
>  drivers/gpu/drm/amd/amdgpu/Makefile | 2 +-
>  drivers/gpu/drm/amd/lib/Makefile| 2 +-
>  drivers/gpu/drm/arm/display/komeda/Makefile | 4 ++--
>  drivers/gpu/drm/i915/gvt/Makefile   | 2 +-
>  drivers/gpu/drm/msm/Makefile| 6 +++---
>  drivers/gpu/drm/nouveau/Kbuild  | 8 
>  6 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile 
> b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 466da59..62bf9da 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -23,7 +23,7 @@
>  # Makefile for the drm device driver.  This driver provides support for the
>  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>  
> -FULL_AMD_PATH=$(src)/..
> +FULL_AMD_PATH=$(srctree)/$(src)/..
>  DISPLAY_FOLDER_NAME=display
>  FULL_AMD_DISPLAY_PATH = $(FULL_AMD_PATH)/$(DISPLAY_FOLDER_NAME)
>  
> diff --git a/drivers/gpu/drm/amd/lib/Makefile 
> b/drivers/gpu/drm/amd/lib/Makefile
> index 6902430..d534992 100644
> --- a/drivers/gpu/drm/amd/lib/Makefile
> +++ b/drivers/gpu/drm/amd/lib/Makefile
> @@ -27,6 +27,6 @@
>  # driver components or later moved to kernel/lib for sharing with
>  # other drivers.
>  
> -ccflags-y := -I$(src)/../include
> +ccflags-y := -I $(srctree)/$(src)/../include
>  
>  obj-$(CONFIG_CHASH) += chash.o
> diff --git a/drivers/gpu/drm/arm/display/komeda/Makefile 
> b/drivers/gpu/drm/arm/display/komeda/Makefile
> index 1b875e5..a72e30c 100644
> --- a/drivers/gpu/drm/arm/display/komeda/Makefile
> +++ b/drivers/gpu/drm/arm/display/komeda/Makefile
> @@ -1,8 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  ccflags-y := \
> - -I$(src)/../include \
> - -I$(src)
> + -I $(srctree)/$(src)/../include \
> + -I $(srctree)/$(src)
>  
>  komeda-y := \
>   komeda_drv.o \

For komeda:

Acked-by: Liviu Dudau 

I'm assuming the series in going to be merged into one go, so I don't
have to take the individual patch in my tree, but if I'm wrong please
let me know.

Best regards,
Liviu

> diff --git a/drivers/gpu/drm/i915/gvt/Makefile 
> b/drivers/gpu/drm/i915/gvt/Makefile
> index 271fb46..ea8324a 100644
> --- a/drivers/gpu/drm/i915/gvt/Makefile
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -5,5 +5,5 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o 
> trace_points.o firmware.o \
>   execlist.o scheduler.o sched_policy.o mmio_context.o cmd_parser.o 
> debugfs.o \
>   fb_decoder.o dmabuf.o page_track.o
>  
> -ccflags-y+= -I$(src) -I$(src)/$(GVT_DIR)
> +ccflags-y+= -I $(srctree)/$(src) -I 
> $(srctree)/$(src)/$(GVT_DIR)/
>  i915-y   += $(addprefix $(GVT_DIR)/, 
> $(GVT_SOURCE))
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 56a70c7..b7b1ebd 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
> -ccflags-y := -Idrivers/gpu/drm/msm
> -ccflags-y += -Idrivers/gpu/drm/msm/disp/dpu1
> -ccflags-$(CONFIG_DRM_MSM_DSI) += -Idrivers/gpu/drm/msm/dsi
> +ccflags-y := -I $(srctree)/$(src)
> +ccflags-y += -I $(srctree)/$(src)/disp/dpu1
> +ccflags-$(CONFIG_DRM_MSM_DSI) += -I $(srctree)/$(src)/dsi
>  
>  msm-y := \
>   adreno/adreno_device.o \
> diff --git a/drivers/gpu/drm/nouveau/Kbuild b/drivers/gpu/drm/nouveau/Kbuild
> index ea3035e..4fae728 100644
> --- a/drivers/gpu/drm/nouveau/Kbuild
> +++ b/drivers/gpu/drm/nouveau/Kbuild
> @@ -1,7 +1,7 @@
>

Re: [Freedreno] [PATCH 2/2] drm/atomic: Create and use __drm_atomic_helper_crtc_reset() everywhere

2018-11-12 Thread Liviu Dudau 
On Mon, Nov 12, 2018 at 04:01:14PM +0100, Maarten Lankhorst wrote:
> We already have __drm_atomic_helper_connector_reset() and
> __drm_atomic_helper_plane_reset(), extend this to crtc as well.
> 
> Most drivers already have a gpu reset hook, correct it.
> Nouveau already implemented its own __drm_atomic_helper_crtc_reset(),
> convert it to the common one.
> 
> Signed-off-by: Maarten Lankhorst 
> Cc: Harry Wentland 
> Cc: Leo Li 
> Cc: Alex Deucher 
> Cc: "Christian König" 
> Cc: "David (ChunMing) Zhou" 
> Cc: David Airlie 
> Cc: Liviu Dudau 
> Cc: Brian Starkey 
> Cc: Mali DP Maintainers 
> Cc: Boris Brezillon 
> Cc: Nicolas Ferre 
> Cc: Alexandre Belloni 
> Cc: Ludovic Desroches 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Sean Paul 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 
> Cc: Philipp Zabel 
> Cc: CK Hu 
> Cc: Matthias Brugger 
> Cc: Rob Clark 
> Cc: Ben Skeggs 
> Cc: Tomi Valkeinen 
> Cc: Laurent Pinchart 
> Cc: Kieran Bingham 
> Cc: Sandy Huang 
> Cc: "Heiko Stübner" 
> Cc: Thierry Reding 
> Cc: Jonathan Hunter 
> Cc: Eric Anholt 
> Cc: VMware Graphics 
> Cc: Sinclair Yeh 
> Cc: Thomas Hellstrom 
> Cc: Tony Cheng 
> Cc: Shirish S 
> Cc: Mikita Lipski 
> Cc: Bhawanpreet Lakha 
> Cc: David Francis 
> Cc: Anthony Koo 
> Cc: Jeykumar Sankaran 
> Cc: Jordan Crouse 
> Cc: Bruce Wang 
> Cc: Sravanthi Kollukuduru 
> Cc: Archit Taneja 
> Cc: Steve Kowalik 
> Cc: Carsten Behling 
> Cc: Haneen Mohammed 
> Cc: Daniel Vetter 
> Cc: Rodrigo Siqueira 
> Cc: Mahesh Kumar 
> Cc: amd-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-ker...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: linux-renesas-...@vger.kernel.org
> Cc: linux-rockc...@lists.infradead.org
> Cc: linux-te...@vger.kernel.org
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 +--
>  drivers/gpu/drm/arm/malidp_crtc.c |  5 +--
>  .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c|  5 +--
>  drivers/gpu/drm/drm_atomic_state_helper.c | 31 ---
>  drivers/gpu/drm/i915/intel_display.c  |  2 +-
>  drivers/gpu/drm/imx/ipuv3-crtc.c  |  5 +--
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c   |  5 +--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 12 ++-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c |  6 +---
>  drivers/gpu/drm/nouveau/dispnv50/head.c   | 13 ++--
>  drivers/gpu/drm/omapdrm/omap_crtc.c   |  7 ++---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c|  4 +--
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  7 +++--
>  drivers/gpu/drm/tegra/dc.c|  5 +--
>  drivers/gpu/drm/vc4/vc4_crtc.c|  8 ++---
>  drivers/gpu/drm/vkms/vkms_crtc.c  |  7 +
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   |  9 +-
>  include/drm/drm_atomic_state_helper.h |  2 ++
>  18 files changed, 56 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 5064768642f3..770a71726cd1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2802,9 +2802,7 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc)
>   if (WARN_ON(!state))
>   return;
>  
> - crtc->state = >base;
> - crtc->state->crtc = crtc;
> -
> + __drm_atomic_helper_crtc_reset(crtc, >base);
>  }
>  
>  static struct drm_crtc_state *
> diff --git a/drivers/gpu/drm/arm/malidp_crtc.c 
> b/drivers/gpu/drm/arm/malidp_crtc.c
> index e1b72782848c..9a924ff27148 100644
> --- a/drivers/gpu/drm/arm/malidp_crtc.c
> +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> @@ -474,10 +474,7 @@ static void malidp_crtc_reset(struct drm_crtc *crtc)
>  
>   kfree(state);
>   state = kzalloc(sizeof(*state), GFP_KERNEL);
> - if (state) {
> - crtc->state = >base;
> - crtc->state->crtc = crtc;
> - }
> + __drm_atomic_helper_crtc_reset(crtc, >base);
>  }
>  
>  static void malidp_crtc_destroy_state(struct drm_crtc *crtc,

For the malidp changes:
Acked-by: Liviu Dudau 

Best regards,
Liviu

> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 96f4082671fe..8084d549c7d1 100644
> --- a/drivers/gpu/

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

2018-02-23 Thread Liviu Dudau 
On Fri, Feb 23, 2018 at 11:39:06AM -0500, Sean Paul wrote:
> On Fri, Feb 23, 2018 at 04:21:05PM +0000, Liviu Dudau wrote:
> > On Fri, Feb 23, 2018 at 10:59:35AM -0500, Sean Paul wrote:
> > > On Fri, Feb 23, 2018 at 08:17:51AM -0500, Rob Clark wrote:
> > > > From: Brian Starkey <brian.star...@arm.com>
> > > > 
> > > > Writeback connectors represent writeback engines which can write the
> > > > CRTC output to a memory framebuffer. Add a writeback connector type and
> > > > related support functions.
> > > > 
> > > > Drivers should initialize a writeback connector with
> > > > drm_writeback_connector_init() which takes care of setting up all the
> > > > writeback-specific details on top of the normal functionality of
> > > > drm_connector_init().
> > > > 
> > > > Writeback connectors have a WRITEBACK_FB_ID property, used to set the
> > > > output framebuffer, and a WRITEBACK_PIXEL_FORMATS blob used to expose 
> > > > the
> > > > supported writeback formats to userspace.
> > > > 
> > > > When a framebuffer is attached to a writeback connector with the
> > > > WRITEBACK_FB_ID property, it is used only once (for the commit in which
> > > > it was included), and userspace can never read back the value of
> > > > WRITEBACK_FB_ID. WRITEBACK_FB_ID can only be set if the connector is
> > > > attached to a CRTC.
> > > > 
> > > > Changes since v1:
> > > >  - Added drm_writeback.c + documentation
> > > >  - Added helper to initialize writeback connector in one go
> > > >  - Added core checks
> > > >  - Squashed into a single commit
> > > >  - Dropped the client cap
> > > >  - Writeback framebuffers are no longer persistent
> > > > 
> > > > Changes since v2:
> > > >  Daniel Vetter:
> > > >  - Subclass drm_connector to drm_writeback_connector
> > > >  - Relax check to allow CRTC to be set without an FB
> > > >  - Add some writeback_ prefixes
> > > >  - Drop PIXEL_FORMATS_SIZE property, as it was unnecessary
> > > >  Gustavo Padovan:
> > > >  - Add drm_writeback_job to handle writeback signalling centrally
> > > > 
> > > > Changes since v3:
> > > >  - Rebased
> > > >  - Rename PIXEL_FORMATS -> WRITEBACK_PIXEL_FORMATS
> > > > 
> > > > Changes since v4:
> > > >  - Added atomic_commit() vfunc to connector helper funcs, so that
> > > >writeback jobs are committed from atomic helpers
> > > > 
> > > > Signed-off-by: Brian Starkey <brian.star...@arm.com>
> > > > [rebased and fixed conflicts]
> > > > Signed-off-by: Mihail Atanassov <mihail.atanas...@arm.com>
> > > > Signed-off-by: Liviu Dudau <liviu.du...@arm.com>
> > > > [rebased and added atomic_commit() vfunc for writeback jobs]
> > > > Signed-off-by: Rob Clark <robdcl...@gmail.com>
> > > > ---
> > > >  Documentation/gpu/drm-kms.rst|   9 ++
> > > >  drivers/gpu/drm/Makefile |   2 +-
> > > >  drivers/gpu/drm/drm_atomic.c | 130 
> > > >  drivers/gpu/drm/drm_atomic_helper.c  |  30 
> > > >  drivers/gpu/drm/drm_connector.c  |   4 +-
> > > >  drivers/gpu/drm/drm_writeback.c  | 257 
> > > > +++
> > > >  include/drm/drm_atomic.h |   3 +
> > > >  include/drm/drm_connector.h  |  13 ++
> > > >  include/drm/drm_mode_config.h|  14 ++
> > > >  include/drm/drm_modeset_helper_vtables.h |  11 ++
> > > >  include/drm/drm_writeback.h  |  89 +++
> > > >  include/uapi/drm/drm_mode.h  |   1 +
> > > >  12 files changed, 561 insertions(+), 2 deletions(-)
> > > >  create mode 100644 drivers/gpu/drm/drm_writeback.c
> > > >  create mode 100644 include/drm/drm_writeback.h
> > > > 
> > > > diff --git a/Documentation/gpu/drm-kms.rst 
> > > > b/Documentation/gpu/drm-kms.rst
> > > > index 2dcf5b42015d..e7590dd2f71e 100644
> > > > --- a/Documentation/gpu/drm-kms.rst
> > > > +++ b/Documentation/gpu/drm-kms.rst
> > > > @@ -370,6 +370,15 @@ Connector Functions Reference
> > > >  .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > > > :export:

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

2018-02-23 Thread Liviu Dudau 
On Fri, Feb 23, 2018 at 11:43:29AM -0500, Sean Paul wrote:
> On Fri, Feb 23, 2018 at 11:25:11AM -0500, Rob Clark wrote:
> > On Fri, Feb 23, 2018 at 10:59 AM, Sean Paul  wrote:
> > >
> > > Have we considered hiding writeback behind a client cap instead?
> > 
> > It is kinda *almost* unneeded, since the connector reports itself as
> > disconnected.
> > 
> > I'm not sure what the reason was to drop the cap, but I think it would
> > be better to have a cap so WB connectors don't show up in, for ex,
> > xrandr
> 
> Yeah, the disconnected hack is kind of gross, IMO. I hate to introduce churn 
> in
> the patch series given that it was initially introduced with the client cap.

Haha, that's the reverse of Daniel's position:

https://lists.freedesktop.org/archives/dri-devel/2016-October/120519.html

> 
> There are also cases where we might want to make writeback unavailable, such 
> as
> when content protection is enabled. In those cases, it's conceivable that we
> might want to use disconnected as a signal to u/s. I suppose we could also 
> just
> fail the check, so most of this is just academic.

Not sure what other hardware out there does, but on Mali DP's case you
would be outputing the protected content by putting the display
processor in secure mode, which automatically disables writeback for us.
Or to put in another way, you don't need a writeback framebuffer if you
are in non-secure mode as you can get access to the framebuffer used for
the plane anyway.

Best regards,
Liviu

> 
> Sean
> 
> 
> > 
> > BR,
> > -R
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

2018-02-23 Thread Liviu Dudau 
On Fri, Feb 23, 2018 at 10:59:35AM -0500, Sean Paul wrote:
> On Fri, Feb 23, 2018 at 08:17:51AM -0500, Rob Clark wrote:
> > From: Brian Starkey <brian.star...@arm.com>
> > 
> > Writeback connectors represent writeback engines which can write the
> > CRTC output to a memory framebuffer. Add a writeback connector type and
> > related support functions.
> > 
> > Drivers should initialize a writeback connector with
> > drm_writeback_connector_init() which takes care of setting up all the
> > writeback-specific details on top of the normal functionality of
> > drm_connector_init().
> > 
> > Writeback connectors have a WRITEBACK_FB_ID property, used to set the
> > output framebuffer, and a WRITEBACK_PIXEL_FORMATS blob used to expose the
> > supported writeback formats to userspace.
> > 
> > When a framebuffer is attached to a writeback connector with the
> > WRITEBACK_FB_ID property, it is used only once (for the commit in which
> > it was included), and userspace can never read back the value of
> > WRITEBACK_FB_ID. WRITEBACK_FB_ID can only be set if the connector is
> > attached to a CRTC.
> > 
> > Changes since v1:
> >  - Added drm_writeback.c + documentation
> >  - Added helper to initialize writeback connector in one go
> >  - Added core checks
> >  - Squashed into a single commit
> >  - Dropped the client cap
> >  - Writeback framebuffers are no longer persistent
> > 
> > Changes since v2:
> >  Daniel Vetter:
> >  - Subclass drm_connector to drm_writeback_connector
> >  - Relax check to allow CRTC to be set without an FB
> >  - Add some writeback_ prefixes
> >  - Drop PIXEL_FORMATS_SIZE property, as it was unnecessary
> >  Gustavo Padovan:
> >  - Add drm_writeback_job to handle writeback signalling centrally
> > 
> > Changes since v3:
> >  - Rebased
> >  - Rename PIXEL_FORMATS -> WRITEBACK_PIXEL_FORMATS
> > 
> > Changes since v4:
> >  - Added atomic_commit() vfunc to connector helper funcs, so that
> >writeback jobs are committed from atomic helpers
> > 
> > Signed-off-by: Brian Starkey <brian.star...@arm.com>
> > [rebased and fixed conflicts]
> > Signed-off-by: Mihail Atanassov <mihail.atanas...@arm.com>
> > Signed-off-by: Liviu Dudau <liviu.du...@arm.com>
> > [rebased and added atomic_commit() vfunc for writeback jobs]
> > Signed-off-by: Rob Clark <robdcl...@gmail.com>
> > ---
> >  Documentation/gpu/drm-kms.rst|   9 ++
> >  drivers/gpu/drm/Makefile |   2 +-
> >  drivers/gpu/drm/drm_atomic.c | 130 
> >  drivers/gpu/drm/drm_atomic_helper.c  |  30 
> >  drivers/gpu/drm/drm_connector.c  |   4 +-
> >  drivers/gpu/drm/drm_writeback.c  | 257 
> > +++
> >  include/drm/drm_atomic.h |   3 +
> >  include/drm/drm_connector.h  |  13 ++
> >  include/drm/drm_mode_config.h|  14 ++
> >  include/drm/drm_modeset_helper_vtables.h |  11 ++
> >  include/drm/drm_writeback.h  |  89 +++
> >  include/uapi/drm/drm_mode.h  |   1 +
> >  12 files changed, 561 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/gpu/drm/drm_writeback.c
> >  create mode 100644 include/drm/drm_writeback.h
> > 
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > index 2dcf5b42015d..e7590dd2f71e 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -370,6 +370,15 @@ Connector Functions Reference
> >  .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > :export:
> >  
> > +Writeback Connectors
> > +
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
> > +  :doc: overview
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_writeback.c
> > +  :export:
> > +
> >  Encoder Abstraction
> >  ===
> >  
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 50093ff4479b..3d708959b224 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -18,7 +18,7 @@ drm-y   :=drm_auth.o drm_bufs.o drm_cache.o \
> > drm_encoder.o drm_mode_object.o drm_property.o \
> > drm_plane.o drm_color_mgmt.o drm_print.o \
> > drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> > -   drm_syncobj.o drm_lease.o
> > +

Re: [Freedreno] [RFC 1/4] drm: Add writeback connector type

2018-02-23 Thread Liviu Dudau 
On Fri, Feb 23, 2018 at 09:24:10AM -0500, Rob Clark wrote:
> On Fri, Feb 23, 2018 at 9:00 AM, Liviu Dudau <liviu.du...@arm.com> wrote:
> > Hi Rob,
> >
> > On Fri, Feb 23, 2018 at 08:17:51AM -0500, Rob Clark wrote:
> >> From: Brian Starkey <brian.star...@arm.com>
> >>
> >> Writeback connectors represent writeback engines which can write the
> >> CRTC output to a memory framebuffer. Add a writeback connector type and
> >> related support functions.
> >>
> >> Drivers should initialize a writeback connector with
> >> drm_writeback_connector_init() which takes care of setting up all the
> >> writeback-specific details on top of the normal functionality of
> >> drm_connector_init().
> >>
> >> Writeback connectors have a WRITEBACK_FB_ID property, used to set the
> >> output framebuffer, and a WRITEBACK_PIXEL_FORMATS blob used to expose the
> >> supported writeback formats to userspace.
> >>
> >> When a framebuffer is attached to a writeback connector with the
> >> WRITEBACK_FB_ID property, it is used only once (for the commit in which
> >> it was included), and userspace can never read back the value of
> >> WRITEBACK_FB_ID. WRITEBACK_FB_ID can only be set if the connector is
> >> attached to a CRTC.
> >
> > [snip]
> >
> >> +static bool create_writeback_properties(struct drm_device *dev)
> >> +{
> >> + struct drm_property *prop;
> >> +
> >> + if (!dev->mode_config.writeback_fb_id_property) {
> >> + prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> >> +   "WRITEBACK_FB_ID",
> >> +   DRM_MODE_OBJECT_FB);
> >> + if (!prop)
> >> + return false;
> >> + dev->mode_config.writeback_fb_id_property = prop;
> >> + }
> >> +
> >> + if (!dev->mode_config.writeback_pixel_formats_property) {
> >> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | 
> >> DRM_MODE_PROP_IMMUTABLE,
> >> +"WRITEBACK_PIXEL_FORMATS", 0);
> >> + if (!prop)
> >> + return false;
> >> + dev->mode_config.writeback_pixel_formats_property = prop;
> >> + }
> >> +
> >> + return true;
> >> +}
> >
> > based on a buildbot warning and the fact that the next patch starts
> > returning -ENOMEM inside the above function, I have this variant in my
> > internal tree that I was preparing to send out once I've got my i-g-t
> > tests in better shape:
> >
> > +static int create_writeback_properties(struct drm_device *dev)
> > +{
> > +   struct drm_property *prop;
> > +
> > +   if (!dev->mode_config.writeback_fb_id_property) {
> > +   prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
> > + "WRITEBACK_FB_ID",
> > + DRM_MODE_OBJECT_FB);
> > +   if (!prop)
> > +   return -ENOMEM;
> > +   dev->mode_config.writeback_fb_id_property = prop;
> > +   }
> > +
> > +   if (!dev->mode_config.writeback_pixel_formats_property) {
> > +   prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | 
> > DRM_MODE_PROP_IMMUTABLE,
> > +  "WRITEBACK_PIXEL_FORMATS", 0);
> > +   if (!prop)
> > +   return -ENOMEM;
> > +   dev->mode_config.writeback_pixel_formats_property = prop;
> > +   }
> > +
> > +   return 0;
> > +}
> >
> >
> > Feel free to use this version in the next update if you're going to send
> > one, otherwise I guess we could be patching it later.
> >
> 
> hmm, I meant to keep my addition of funcs->atomic_commit() as a
> separate fixup patch so it would be easier for you to fold back into
> your patchset, but accidentally squashed it at some point and was too
> lazy to split it out again.  Sorry about that.

I'm not too fussed about who pushes Brian's framework patches into
drm-next so I don't mind at all you merging your addition. Just wanted
to make sure we're on the same page (didn't know you're going to send
your series this week).

I also feel I need to appologise for my delay in getting the i-g-t
patches

[Freedreno] [PATCH] drm/msm/hdmi: Use bitwise operators when building register values

2017-06-15 Thread Liviu Dudau 
Commit c0c0d9eeeb8d ("drm/msm: hdmi audio support") uses logical
OR operators to build up a value to be written in the
REG_HDMI_AUDIO_INFO0 and REG_HDMI_AUDIO_INFO1 registers when it
should have used bitwise operators.

Signed-off-by: Liviu Dudau <liviu.du...@arm.com>
Fixes: c0c0d9eeeb8d ("drm/msm: hdmi audio support")
---
 drivers/gpu/drm/msm/hdmi/hdmi_audio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_audio.c 
b/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
index 8177e8511afd..9c34b91ae329 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
@@ -175,10 +175,10 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
/* configure infoframe: */
hdmi_audio_infoframe_pack(info, buf, sizeof(buf));
hdmi_write(hdmi, REG_HDMI_AUDIO_INFO0,
-   (buf[3] <<  0) || (buf[4] <<  8) ||
-   (buf[5] << 16) || (buf[6] << 24));
+   (buf[3] <<  0) | (buf[4] <<  8) |
+   (buf[5] << 16) | (buf[6] << 24));
hdmi_write(hdmi, REG_HDMI_AUDIO_INFO1,
-   (buf[7] <<  0) || (buf[8] << 8));
+   (buf[7] <<  0) | (buf[8] << 8));
 
hdmi_write(hdmi, REG_HDMI_GC, 0);
 
-- 
2.13.1

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v3] drm: Add DRM_MODE_ROTATE_ and DRM_MODE_REFLECT_ to UAPI

2017-05-18 Thread Liviu Dudau 
_MODE_ROTATE_MASK)
> + val |= ilog2(plane->state->rotation & DRM_MODE_ROTATE_MASK) <<
>  LAYER_ROT_OFFSET;
> - if (plane->state->rotation & DRM_REFLECT_X)
> + if (plane->state->rotation & DRM_MODE_REFLECT_X)
>   val |= LAYER_H_FLIP;
> - if (plane->state->rotation & DRM_REFLECT_Y)
> + if (plane->state->rotation & DRM_MODE_REFLECT_Y)
>   val |= LAYER_V_FLIP;
>  
>   /*
> @@ -370,8 +370,8 @@ int malidp_de_planes_init(struct drm_device *drm)
>   struct malidp_plane *plane = NULL;
>   enum drm_plane_type plane_type;
>   unsigned long crtcs = 1 << drm->mode_config.num_crtc;
> - unsigned long flags = DRM_ROTATE_0 | DRM_ROTATE_90 | DRM_ROTATE_180 |
> -   DRM_ROTATE_270 | DRM_REFLECT_X | DRM_REFLECT_Y;
> + unsigned long flags = DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_90 | 
> DRM_MODE_ROTATE_180 |
> +   DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X | 
> DRM_MODE_REFLECT_Y;
>   u32 *formats;
>   int ret, i, j, n;
>  
> @@ -420,7 +420,7 @@ int malidp_de_planes_init(struct drm_device *drm)
>   continue;
>   }
>  
> - drm_plane_create_rotation_property(>base, DRM_ROTATE_0, 
> flags);
> + drm_plane_create_rotation_property(>base, 
> DRM_MODE_ROTATE_0, flags);
>   malidp_hw_write(malidp->dev, MALIDP_ALPHA_LUT,
>   plane->layer->base + MALIDP_LAYER_COMPOSE);
>   }

For the mali-dp part:

Acked-by: Liviu Dudau <liviu.du...@arm.com>

Thanks,
Liviu


> diff --git a/drivers/gpu/drm/armada/armada_overlay.c 
> b/drivers/gpu/drm/armada/armada_overlay.c
> index 424e465ff407..e9a29df4b443 100644
> --- a/drivers/gpu/drm/armada/armada_overlay.c
> +++ b/drivers/gpu/drm/armada/armada_overlay.c
> @@ -125,7 +125,7 @@ armada_ovl_plane_update(struct drm_plane *plane, struct 
> drm_crtc *crtc,
>src_x, src_y, src_w, src_h);
>  
>   ret = drm_plane_helper_check_update(plane, crtc, fb, , , ,
> - DRM_ROTATE_0,
> + DRM_MODE_ROTATE_0,
>   0, INT_MAX, true, false, );
>   if (ret)
>   return ret;
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> index 29cc10d053eb..1124200bb280 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> @@ -678,8 +678,8 @@ static int atmel_hlcdc_plane_atomic_check(struct 
> drm_plane *p,
>   if (!state->bpp[i])
>   return -EINVAL;
>  
> - switch (state->base.rotation & DRM_ROTATE_MASK) {
> - case DRM_ROTATE_90:
> + switch (state->base.rotation & DRM_MODE_ROTATE_MASK) {
> + case DRM_MODE_ROTATE_90:
>   offset = ((y_offset + state->src_y + patched_src_w - 1) 
> /
> ydiv) * fb->pitches[i];
>   offset += ((x_offset + state->src_x) / xdiv) *
> @@ -688,7 +688,7 @@ static int atmel_hlcdc_plane_atomic_check(struct 
> drm_plane *p,
> fb->pitches[i];
>   state->pstride[i] = -fb->pitches[i] - state->bpp[i];
>   break;
> - case DRM_ROTATE_180:
> + case DRM_MODE_ROTATE_180:
>   offset = ((y_offset + state->src_y + patched_src_h - 1) 
> /
> ydiv) * fb->pitches[i];
>   offset += ((x_offset + state->src_x + patched_src_w - 
> 1) /
> @@ -697,7 +697,7 @@ static int atmel_hlcdc_plane_atomic_check(struct 
> drm_plane *p,
>  state->bpp[i]) - fb->pitches[i];
>   state->pstride[i] = -2 * state->bpp[i];
>   break;
> - case DRM_ROTATE_270:
> + case DRM_MODE_ROTATE_270:
>   offset = ((y_offset + state->src_y) / ydiv) *
>fb->pitches[i];
>   offset += ((x_offset + state->src_x + patched_src_h - 
> 1) /
> @@ -707,7 +707,7 @@ static int atmel_hlcdc_plane_atomic_check(struct 
> drm_plane *p,
> (2 * state->bpp[i]);
>   state->pstride[i] = fb->pitches[i] - state->bpp[i];
>   break;
> -