Hi Mahesh,
Thank you for the patch.
On Friday, 13 July 2018 16:59:42 EEST Mahesh Kumar wrote:
> This patch implements get_crc_sources callback, which returns list of
> all the crc sources supported by driver in current platform.
>
> Changes Since V1:
> - move sources list per-crtc
> - init sources-list only for gen3
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 96 ++++++++++++++++++++++++++++++-
> drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 3 ++
> 2 files changed, 98 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 6a29055a4ab0..bbe417e93fe3
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -691,6 +691,79 @@ static const struct drm_crtc_helper_funcs
> crtc_helper_funcs = { .atomic_disable = rcar_du_crtc_atomic_disable,
> };
>
> +static void rcar_du_crtc_crc_sources_list_init(struct rcar_du_crtc *rcrtc)
> +{
> + struct rcar_du_device *rcdu = rcrtc->group->dev;
> + struct drm_device *dev = rcrtc->crtc.dev;
> + struct drm_crtc *crtc = &rcrtc->crtc;
> + struct drm_plane *plane;
> + unsigned int count;
> + const char **sources;
> + u32 plane_mask;
> + int i = 0;
i never takes negative values, it can be an unsigned int.
> + /* CRC available only on Gen3 HW */
Please capitalize sentences and end them with a period in comments to match
the driver's style. This applies to other locations in this patch.
> + if (rcdu->info->gen < 3)
> + goto fail;
You can just return here, sources_count and sources are initialized to 0 when
the rcar_du_crtc structure is allocated.
> + drm_for_each_plane(plane, dev) {
> + if (drm_crtc_mask(crtc) & plane->possible_crtcs) {
> + count++;
> + plane_mask |= drm_plane_mask(plane);
> + }
> + }
You can instead iterate over the planes of the associated VSP (hardware
composer).
/* Reserve 1 for "auto" source. */
count = rcrtc->vsp->num_planes + 1;
and get rid of plane_mask.
> + /* reserve 1 for "auto" source */
> + count += 1;
> + sources = kmalloc_array(count, sizeof(char *), GFP_KERNEL);
s/sizeof(char *)/sizeof(*sources)/
> + if (!sources)
> + goto fail;
> +
> + sources[i] = kstrdup("auto", GFP_KERNEL);
> + if (!sources[i])
> + goto fail_no_mem;
> +
> + i++;
> + drm_for_each_plane_mask(plane, dev, plane_mask) {
> + char name[16];
> +
> + sprintf(name, "plane%d", plane->base.id);
The ID is an unsigned integer, you should use %u.
> + sources[i] = kstrdup(name, GFP_KERNEL);
> + if (!sources[i])
> + goto fail_no_mem;
As there will be a single error label, you can just name it "error".
> + i++;
> + }
You can iterate over the VSP planes here too.
for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
struct drm_plane *plane = &rcrtc->vsp->planes[i].plane;
char name[16];
sprintf(name, "plane%u", plane->base.id);
sources[i+1] = kstrdup(name, GFP_KERNEL);
if (!sources[i+1])
goto error;
}
> + rcrtc->sources = sources;
> + rcrtc->sources_count = count;
> + return;
> +
> +fail_no_mem:
> + while (i > 0) {
> + i--;
> + kfree(sources[i]);
> + }
You'll have to adapt it based on the code above.
> + kfree(sources);
> +fail:
> + rcrtc->sources = NULL;
> + rcrtc->sources_count = 0;
> +}
> +
> +static void rcar_du_crtc_crc_sources_list_uninit(struct rcar_du_crtc
> *rcrtc)
> +{
> + unsigned int i;
> +
> + if (!rcrtc->sources)
> + return;
> +
> + for (i = 0; i < rcrtc->sources_count; i++)
> + kfree(rcrtc->sources[i]);
> + kfree(rcrtc->sources);
> +
> + rcrtc->sources = NULL;
> + rcrtc->sources_count = 0;
> +}
> +
> static struct drm_crtc_state *
> rcar_du_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
> {
> @@ -717,6 +790,15 @@ static void rcar_du_crtc_atomic_destroy_state(struct
> drm_crtc *crtc, kfree(to_rcar_crtc_state(state));
> }
>
> +static void rcar_du_crtc_cleanup(struct drm_crtc *crtc)
> +{
> + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +
> + rcar_du_crtc_crc_sources_list_uninit(rcrtc);
> +
> + return drm_crtc_cleanup(crtc);
> +}
> +
> static void rcar_du_crtc_reset(struct drm_crtc *crtc)
> {
> struct rcar_du_crtc_state *state;
> @@ -811,6 +893,15 @@ static int rcar_du_crtc_verify_crc_source(struct
> drm_crtc *crtc, return 0;
> }
>
> +const char *const *rcar_du_crtc_get_crc_sources(struct drm_crtc *crtc,
> + size_t *count)
> +{
> + struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +
> + *count = rcrtc->sources_count;
> + return (const char * const*)rcrtc->sources;
Shouldn't you declare rcrtc->sources as a const char * const * field instead
of casting it here ?
> +}
> +
> static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
> const char *source_name)
> {
> @@ -881,7 +972,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen2 = {
>
> static const struct drm_crtc_funcs crtc_funcs_gen3 = {
> .reset = rcar_du_crtc_reset,
> - .destroy = drm_crtc_cleanup,
> + .destroy = rcar_du_crtc_cleanup,
> .set_config = drm_atomic_helper_set_config,
> .page_flip = drm_atomic_helper_page_flip,
> .atomic_duplicate_state = rcar_du_crtc_atomic_duplicate_state,
> @@ -890,6 +981,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
> .disable_vblank = rcar_du_crtc_disable_vblank,
> .set_crc_source = rcar_du_crtc_set_crc_source,
> .verify_crc_source = rcar_du_crtc_verify_crc_source,
> + .get_crc_sources = rcar_du_crtc_get_crc_sources,
> };
>
> /*
> ---------------------------------------------------------------------------
> -- @@ -1028,5 +1120,7 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp,
> unsigned int swindex, return ret;
> }
>
> + rcar_du_crtc_crc_sources_list_init(rcrtc);
> +
> return 0;
> }
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index 7680cb2636c8..0cd0c1655beb
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -67,6 +67,9 @@ struct rcar_du_crtc {
> struct rcar_du_group *group;
> struct rcar_du_vsp *vsp;
> unsigned int vsp_pipe;
> +
> + const char **sources;
> + unsigned int sources_count;
> };
>
> #define to_rcar_crtc(c) container_of(c, struct rcar_du_crtc, crtc)
--
Regards,
Laurent Pinchart
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
