Re: [PATCH v3 2/7] drm/tegra: Add plane support
On 02/18/2013 03:01 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 02:55:04PM +0800, Mark Zhang wrote:
>> On 02/18/2013 02:40 PM, Thierry Reding wrote:
>>> On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
On 02/14/2013 12:05 AM, Thierry Reding wrote:
> Add support for the B and C planes which support RGB and YUV pixel
> formats and can be used as overlays or hardware cursor. Currently
> only 32-bit RGBA pixel formats are advertised.
>
> Signed-off-by: Thierry Reding
[...]
> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc
> *dc)
> +{
> + unsigned int i;
> + int err = 0;
> +
> + for (i = 0; i < 2; i++) {
> + struct tegra_plane *plane;
> +
> + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
Using "devm_kzalloc" here seems like not a good idea. Everytime plane
disable or crtc disable, we should free "struct tegra_plane" which is
allocated here. But the memory which devm_kzalloc allocates is only
freed when the driver detach. This makes lots of memory can't be
recycled when the plane enable/disable frequently.
>>>
>>> Why would we want to do that? I don't think doing so would even work,
>>> since the planes are resources that need to be registered with the DRM
>>> core and therefore can't be allocated on-demand.
>>>
>>
>> I'm wondering if we add power management codes in the future, the
>> crtc/plane will be disabled/enabled frequently, e.g: system going to
>> suspend state then resume.
>
> In that case it makes even less sense to allocate and deallocate the
> plane every time around. However, any optimizations aside, the allocated
> memory is passed into the core via drm_plane_init(), which registers
> that plane with the given CRTC. So if we deallocate the memory when the
> plane when it is disabled, we'd have to make sure to remove it from the
> core as well. However that would also mean that it disappears from the
> list of planes supported by the CRTC and therefore we wouldn't be able
> to use it again.
>
Alright, I mixed up the "disable" & "remove" of planes and crtcs. You're
right. Just forget what I said in this thread.
Mark
> Thierry
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/7] drm/tegra: Add plane support
On Mon, Feb 18, 2013 at 02:55:04PM +0800, Mark Zhang wrote:
> On 02/18/2013 02:40 PM, Thierry Reding wrote:
> > On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
> >> On 02/14/2013 12:05 AM, Thierry Reding wrote:
> >>> Add support for the B and C planes which support RGB and YUV pixel
> >>> formats and can be used as overlays or hardware cursor. Currently
> >>> only 32-bit RGBA pixel formats are advertised.
> >>>
> >>> Signed-off-by: Thierry Reding
> >> [...]
> >>> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc
> >>> *dc)
> >>> +{
> >>> + unsigned int i;
> >>> + int err = 0;
> >>> +
> >>> + for (i = 0; i < 2; i++) {
> >>> + struct tegra_plane *plane;
> >>> +
> >>> + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
> >>
> >> Using "devm_kzalloc" here seems like not a good idea. Everytime plane
> >> disable or crtc disable, we should free "struct tegra_plane" which is
> >> allocated here. But the memory which devm_kzalloc allocates is only
> >> freed when the driver detach. This makes lots of memory can't be
> >> recycled when the plane enable/disable frequently.
> >
> > Why would we want to do that? I don't think doing so would even work,
> > since the planes are resources that need to be registered with the DRM
> > core and therefore can't be allocated on-demand.
> >
>
> I'm wondering if we add power management codes in the future, the
> crtc/plane will be disabled/enabled frequently, e.g: system going to
> suspend state then resume.
In that case it makes even less sense to allocate and deallocate the
plane every time around. However, any optimizations aside, the allocated
memory is passed into the core via drm_plane_init(), which registers
that plane with the given CRTC. So if we deallocate the memory when the
plane when it is disabled, we'd have to make sure to remove it from the
core as well. However that would also mean that it disappears from the
list of planes supported by the CRTC and therefore we wouldn't be able
to use it again.
Thierry
pgplzwLTjYpXR.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/7] drm/tegra: Add plane support
On 02/18/2013 02:40 PM, Thierry Reding wrote:
> On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
>> On 02/14/2013 12:05 AM, Thierry Reding wrote:
>>> Add support for the B and C planes which support RGB and YUV pixel
>>> formats and can be used as overlays or hardware cursor. Currently
>>> only 32-bit RGBA pixel formats are advertised.
>>>
>>> Signed-off-by: Thierry Reding
>> [...]
>>> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
>>> +{
>>> + unsigned int i;
>>> + int err = 0;
>>> +
>>> + for (i = 0; i < 2; i++) {
>>> + struct tegra_plane *plane;
>>> +
>>> + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
>>
>> Using "devm_kzalloc" here seems like not a good idea. Everytime plane
>> disable or crtc disable, we should free "struct tegra_plane" which is
>> allocated here. But the memory which devm_kzalloc allocates is only
>> freed when the driver detach. This makes lots of memory can't be
>> recycled when the plane enable/disable frequently.
>
> Why would we want to do that? I don't think doing so would even work,
> since the planes are resources that need to be registered with the DRM
> core and therefore can't be allocated on-demand.
>
I'm wondering if we add power management codes in the future, the
crtc/plane will be disabled/enabled frequently, e.g: system going to
suspend state then resume.
> Thierry
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/7] drm/tegra: Add plane support
On Mon, Feb 18, 2013 at 02:06:56PM +0800, Mark Zhang wrote:
> On 02/14/2013 12:05 AM, Thierry Reding wrote:
> > Add support for the B and C planes which support RGB and YUV pixel
> > formats and can be used as overlays or hardware cursor. Currently
> > only 32-bit RGBA pixel formats are advertised.
> >
> > Signed-off-by: Thierry Reding
> [...]
> > +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
> > +{
> > + unsigned int i;
> > + int err = 0;
> > +
> > + for (i = 0; i < 2; i++) {
> > + struct tegra_plane *plane;
> > +
> > + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
>
> Using "devm_kzalloc" here seems like not a good idea. Everytime plane
> disable or crtc disable, we should free "struct tegra_plane" which is
> allocated here. But the memory which devm_kzalloc allocates is only
> freed when the driver detach. This makes lots of memory can't be
> recycled when the plane enable/disable frequently.
Why would we want to do that? I don't think doing so would even work,
since the planes are resources that need to be registered with the DRM
core and therefore can't be allocated on-demand.
Thierry
pgppxnamVJgvv.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/7] drm/tegra: Add plane support
On 02/14/2013 12:05 AM, Thierry Reding wrote:
> Add support for the B and C planes which support RGB and YUV pixel
> formats and can be used as overlays or hardware cursor. Currently
> only 32-bit RGBA pixel formats are advertised.
>
> Signed-off-by: Thierry Reding
[...]
> +static int tegra_dc_add_planes(struct drm_device *drm, struct tegra_dc *dc)
> +{
> + unsigned int i;
> + int err = 0;
> +
> + for (i = 0; i < 2; i++) {
> + struct tegra_plane *plane;
> +
> + plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
Using "devm_kzalloc" here seems like not a good idea. Everytime plane
disable or crtc disable, we should free "struct tegra_plane" which is
allocated here. But the memory which devm_kzalloc allocates is only
freed when the driver detach. This makes lots of memory can't be
recycled when the plane enable/disable frequently.
> + if (!plane)
> + return -ENOMEM;
> +
> + plane->index = 1 + i;
> +
> + err = drm_plane_init(drm, &plane->base, 1 << dc->pipe,
> + &tegra_plane_funcs, plane_formats,
> + ARRAY_SIZE(plane_formats), false);
> + if (err < 0)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> static const struct drm_crtc_funcs tegra_crtc_funcs = {
> .set_config = drm_crtc_helper_set_config,
> .destroy = drm_crtc_cleanup,
> };
>
> -static void tegra_crtc_dpms(struct drm_crtc *crtc, int mode)
> +static void tegra_crtc_disable(struct drm_crtc *crtc)
> {
> + struct drm_device *drm = crtc->dev;
> + struct drm_plane *plane;
> +
> + list_for_each_entry(plane, &drm->mode_config.plane_list, head) {
> + if (plane->crtc == crtc) {
> + tegra_plane_disable(plane);
> + plane->crtc = NULL;
> +
> + if (plane->fb) {
> + drm_framebuffer_unreference(plane->fb);
> + plane->fb = NULL;
> + }
> + }
> + }
If what I mentioned above(about using devm_kzalloc to allocate "struct
tegra_plane") is correct, we need to free "struct tegra_plane" here.
> }
>
> static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
> @@ -46,10 +144,11 @@ static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
> return true;
> }
>
[...]
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
