Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:23PM +0200, Jacopo Mondi wrote:
> Implement device tree parsing to collect the available CMM units and
> store them in the group to be later enabled at start up time.
> 
> Define a new feature to let each SoC claim support for CMM.
> 
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  4 ++
>  drivers/gpu/drm/rcar-du/rcar_du_group.h |  2 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 68 +++++++++++++++++++++++++
>  3 files changed, 74 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h 
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index 1327cd0df90a..aac916a7a46c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -13,6 +13,7 @@
>  #include <linux/kernel.h>
>  #include <linux/wait.h>
>  
> +#include "rcar_du_cmm.h"
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_group.h"
>  #include "rcar_du_vsp.h"
> @@ -28,6 +29,7 @@ struct rcar_du_encoder;
>  #define RCAR_DU_FEATURE_VSP1_SOURCE  BIT(1)  /* Has inputs from VSP1 */
>  #define RCAR_DU_FEATURE_INTERLACED   BIT(2)  /* HW supports interlaced */
>  #define RCAR_DU_FEATURE_TVM_SYNC     BIT(3)  /* Has TV switch/sync modes */
> +#define RCAR_DU_FEATURE_CMM          BIT(4)  /* Has CCM */
>  
>  #define RCAR_DU_QUIRK_ALIGN_128B     BIT(0)  /* Align pitches to 128 bytes */
>  
> @@ -69,6 +71,7 @@ struct rcar_du_device_info {
>  
>  #define RCAR_DU_MAX_CRTCS            4
>  #define RCAR_DU_MAX_GROUPS           DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
> +#define RCAR_DU_MAX_CMMS             4
>  #define RCAR_DU_MAX_VSPS             4
>  
>  struct rcar_du_device {
> @@ -85,6 +88,7 @@ struct rcar_du_device {
>       struct rcar_du_encoder *encoders[RCAR_DU_OUTPUT_MAX];
>  
>       struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
> +     struct rcar_du_cmm cmms[RCAR_DU_MAX_CMMS];
>       struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
>  
>       struct {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h 
> b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> index 87950c1f6a52..b0c1466593a3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> @@ -22,6 +22,7 @@ struct rcar_du_device;
>   * @mmio_offset: registers offset in the device memory map
>   * @index: group index
>   * @channels_mask: bitmask of populated DU channels in this group
> + * @cmms_mask: bitmask of enabled CMMs in this group
>   * @num_crtcs: number of CRTCs in this group (1 or 2)
>   * @use_count: number of users of the group (rcar_du_group_(get|put))
>   * @used_crtcs: number of CRTCs currently in use
> @@ -37,6 +38,7 @@ struct rcar_du_group {
>       unsigned int index;
>  
>       unsigned int channels_mask;
> +     unsigned int cmms_mask;
>       unsigned int num_crtcs;
>       unsigned int use_count;
>       unsigned int used_crtcs;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index adbc4f5d8fc5..684a60feac5c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -614,6 +614,64 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
>       return ret;
>  }
>  
> +static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
> +{
> +     const struct device_node *np = rcdu->dev->of_node;
> +     unsigned int cells;
> +     unsigned int i;
> +     int ret;
> +
> +     /*
> +      * Make sure the DT 'cmms' property is well formed and populate
> +      * the list of enabled CMM.
> +      */
> +     cells = of_property_count_u32_elems(np, "cmms");
> +     if (cells % 2 || cells > rcdu->num_crtcs * 2) {
> +             dev_err(rcdu->dev, "invalid 'cmms' property format\n");
> +             return -EINVAL;
> +     }
> +
> +     for (i = 0; i < cells / 2; ++i) {
> +             struct of_phandle_args args;
> +             struct rcar_du_group *rgrp;
> +             unsigned int crtc;
> +
> +             ret = of_parse_phandle_with_fixed_args(np, "cmms", 1, i,
> +                                                    &args);
> +             if (ret < 0) {
> +                     dev_err(rcdu->dev, "failed to parse 'cmms' property\n");
> +                     goto error;
> +             }
> +
> +             crtc = args.args[0];
> +             if (crtc >= rcdu->num_crtcs ||
> +                 !(rcdu->info->channels_mask & BIT(crtc))) {
> +                     dev_err(rcdu->dev,
> +                             "invalid DU channel %u in 'cmms' property\n",
> +                             crtc);
> +                     goto error;
> +             }
> +
> +             rcdu->cmms[i].np = args.np;

How about storing the struct device pointer instead (using
of_find_device_by_node()) ? You should add a rcar_cmm_init() function to
the CMM API, similar to vsp1_du_init().

> +             rcdu->cmms[i].crtc = crtc;
> +
> +             /*
> +              * CMMs are activated by the DU group: store the active CMMs
> +              * indexes in the group.
> +              */
> +             rgrp = &rcdu->groups[crtc / 2];
> +             rgrp->cmms_mask |= BIT(crtc % 2);
> +     }

I think you can simplify this by dropping the index from renesas,cmms.
You can simply loop over the CRTCs and get the phandle corresponding to
the CRTC index.

> +
> +     return 0;
> +
> +error:
> +     for (; i > 0; --i)
> +             of_node_put(rcdu->cmms[i - 1].np);
> +
> +     return ret;
> +}
> +
>  int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  {
>       static const unsigned int mmio_offsets[] = {
> @@ -680,6 +738,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>                                  & GENMASK(1, 0);
>               rgrp->num_crtcs = hweight8(rgrp->channels_mask);
>  
> +             /* The active CMMs mask will be later populated. */
> +             rgrp->cmms_mask = 0;
> +

The structures are initialised to 0 when allocated so this isn't
strictly required.

>               /*
>                * If we have more than one CRTCs in this group pre-associate
>                * the low-order planes with CRTC 0 and the high-order planes
> @@ -704,6 +765,13 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>                       return ret;
>       }
>  
> +     /* Initialize the Color Management Modules. */
> +     if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
> +             ret = rcar_du_cmm_init(rcdu);
> +             if (ret < 0)
> +                     return ret;
> +     }
> +
>       /* Create the CRTCs. */
>       for (swindex = 0, hwindex = 0; swindex < rcdu->num_crtcs; ++hwindex) {
>               struct rcar_du_group *rgrp;

-- 
Regards,

Laurent Pinchart

Reply via email to