| Message ID | 20180713135942.25061-11-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)
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)
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; + + /* CRC available only on Gen3 HW */ + if (rcdu->info->gen < 3) + goto fail; + + drm_for_each_plane(plane, dev) { + if (drm_crtc_mask(crtc) & plane->possible_crtcs) { + count++; + plane_mask |= drm_plane_mask(plane); + } + } + + /* reserve 1 for "auto" source */ + count += 1; + sources = kmalloc_array(count, sizeof(char *), GFP_KERNEL); + 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); + sources[i] = kstrdup(name, GFP_KERNEL); + if (!sources[i]) + goto fail_no_mem; + i++; + } + + rcrtc->sources = sources; + rcrtc->sources_count = count; + return; + +fail_no_mem: + while (i > 0) { + i--; + kfree(sources[i]); + } + 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; +} + 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)
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(-)