Re: [PATCH v4 10/10] drm/rcar-du/crc: Implement get_crc_sources callback

From: "Kumar, Mahesh" <mahesh1.kumar@intel.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v4 10/10] drm/rcar-du/crc: Implement get_crc_sources callback
Date: Thu, 19 Jul 2018 16:54:25 +0530	[thread overview]
Message-ID: <4e262a2d-6376-cdc6-6f39-231aa1529be2@intel.com> (raw)
In-Reply-To: <1600855.omIG2KBUNv@avalon>

Hi Laurent!

Thanks for the review. :)

will update patch and resubmit

-Mahesh

On 7/19/2018 4:42 PM, Laurent Pinchart wrote:
> 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@intel.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   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)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx