[Intel-gfx] [PATCH v4 10/10] drm/rcar-du/crc: Implement get_crc_sources callback
Kumar, Mahesh
mahesh1.kumar at intel.com
Thu Jul 19 11:24:25 UTC 2018
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 at intel.com>
>> Cc: Laurent Pinchart <laurent.pinchart at 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)
More information about the Intel-gfx
mailing list