* [PATCH] drm/sti: forbid plane on several mixer
@ 2016-09-14 11:40 Vincent Abriou
2016-09-15 13:06 ` Benjamin Gaignard
2016-09-15 14:27 ` Ville Syrjälä
0 siblings, 2 replies; 6+ messages in thread
From: Vincent Abriou @ 2016-09-14 11:40 UTC (permalink / raw)
To: dri-devel; +Cc: Vincent Abriou, Fabien Dessenne, kernel
When a plane is going to be enabled we re-evaluate the possible crtcs
for the associated drm plane. Only the crtc on which the plane should be
displayed is considered possible until the plane is disabled.
Indeed STI hardware does not allow to dynamically change
the plane's crtc/mixer assignment when the plane is in use (gdp is
running).
Signed-off-by: Vincent Abriou <vincent.abriou@st.com>
---
drivers/gpu/drm/sti/sti_gdp.c | 15 +++++++++++++++
drivers/gpu/drm/sti/sti_plane.h | 2 ++
2 files changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
index 3fc62c1..f7cd671 100644
--- a/drivers/gpu/drm/sti/sti_gdp.c
+++ b/drivers/gpu/drm/sti/sti_gdp.c
@@ -71,6 +71,9 @@ static struct gdp_format_to_str {
#define GDP_NODE_NB_BANK 2
#define GDP_NODE_PER_FIELD 2
+#define MAIN_CRTC_MASK BIT(0)
+#define AUX_CRTC_MASK BIT(1)
+
struct sti_gdp_node {
u32 gam_gdp_ctl;
u32 gam_gdp_agc;
@@ -690,6 +693,12 @@ static int sti_gdp_atomic_check(struct drm_plane *drm_plane,
}
}
+ /* re-evaluate the possible crtcs */
+ if (mixer->id == STI_MIXER_MAIN)
+ drm_plane->possible_crtcs = MAIN_CRTC_MASK;
+ else
+ drm_plane->possible_crtcs = AUX_CRTC_MASK;
+
DRM_DEBUG_KMS("CRTC:%d (%s) drm plane:%d (%s)\n",
crtc->base.id, sti_mixer_to_str(mixer),
drm_plane->base.id, sti_plane_to_str(plane));
@@ -846,6 +855,9 @@ static void sti_gdp_atomic_disable(struct drm_plane *drm_plane,
{
struct sti_plane *plane = to_sti_plane(drm_plane);
+ /* restore possible crtcs value with the initial value */
+ drm_plane->possible_crtcs = plane->init_possible_crtcs;
+
if (!drm_plane->crtc) {
DRM_DEBUG_DRIVER("drm plane:%d not enabled\n",
drm_plane->base.id);
@@ -917,6 +929,9 @@ struct drm_plane *sti_gdp_create(struct drm_device *drm_dev,
sti_gdp_init(gdp);
+ /* store the initial value of possible crtcs */
+ gdp->plane.init_possible_crtcs = possible_crtcs;
+
res = drm_universal_plane_init(drm_dev, &gdp->plane.drm_plane,
possible_crtcs,
&sti_gdp_plane_helpers_funcs,
diff --git a/drivers/gpu/drm/sti/sti_plane.h b/drivers/gpu/drm/sti/sti_plane.h
index ce3e8d6..70c5312 100644
--- a/drivers/gpu/drm/sti/sti_plane.h
+++ b/drivers/gpu/drm/sti/sti_plane.h
@@ -66,12 +66,14 @@ struct sti_fps_info {
* @plane: drm plane it is bound to (if any)
* @desc: plane type & id
* @status: to know the status of the plane
+ * @init_possile_crtcs: store the initial possible crtc value
* @fps_info: frame per second info
*/
struct sti_plane {
struct drm_plane drm_plane;
enum sti_plane_desc desc;
enum sti_plane_status status;
+ u32 init_possible_crtcs;
struct sti_fps_info fps_info;
};
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sti: forbid plane on several mixer
2016-09-14 11:40 [PATCH] drm/sti: forbid plane on several mixer Vincent Abriou
@ 2016-09-15 13:06 ` Benjamin Gaignard
2016-09-15 14:27 ` Ville Syrjälä
1 sibling, 0 replies; 6+ messages in thread
From: Benjamin Gaignard @ 2016-09-15 13:06 UTC (permalink / raw)
To: Vincent Abriou; +Cc: Fabien Dessenne, dri-devel@lists.freedesktop.org, kernel
Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
2016-09-14 13:40 GMT+02:00 Vincent Abriou <vincent.abriou@st.com>:
> When a plane is going to be enabled we re-evaluate the possible crtcs
> for the associated drm plane. Only the crtc on which the plane should be
> displayed is considered possible until the plane is disabled.
> Indeed STI hardware does not allow to dynamically change
> the plane's crtc/mixer assignment when the plane is in use (gdp is
> running).
>
> Signed-off-by: Vincent Abriou <vincent.abriou@st.com>
> ---
> drivers/gpu/drm/sti/sti_gdp.c | 15 +++++++++++++++
> drivers/gpu/drm/sti/sti_plane.h | 2 ++
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
> index 3fc62c1..f7cd671 100644
> --- a/drivers/gpu/drm/sti/sti_gdp.c
> +++ b/drivers/gpu/drm/sti/sti_gdp.c
> @@ -71,6 +71,9 @@ static struct gdp_format_to_str {
> #define GDP_NODE_NB_BANK 2
> #define GDP_NODE_PER_FIELD 2
>
> +#define MAIN_CRTC_MASK BIT(0)
> +#define AUX_CRTC_MASK BIT(1)
> +
> struct sti_gdp_node {
> u32 gam_gdp_ctl;
> u32 gam_gdp_agc;
> @@ -690,6 +693,12 @@ static int sti_gdp_atomic_check(struct drm_plane *drm_plane,
> }
> }
>
> + /* re-evaluate the possible crtcs */
> + if (mixer->id == STI_MIXER_MAIN)
> + drm_plane->possible_crtcs = MAIN_CRTC_MASK;
> + else
> + drm_plane->possible_crtcs = AUX_CRTC_MASK;
> +
> DRM_DEBUG_KMS("CRTC:%d (%s) drm plane:%d (%s)\n",
> crtc->base.id, sti_mixer_to_str(mixer),
> drm_plane->base.id, sti_plane_to_str(plane));
> @@ -846,6 +855,9 @@ static void sti_gdp_atomic_disable(struct drm_plane *drm_plane,
> {
> struct sti_plane *plane = to_sti_plane(drm_plane);
>
> + /* restore possible crtcs value with the initial value */
> + drm_plane->possible_crtcs = plane->init_possible_crtcs;
> +
> if (!drm_plane->crtc) {
> DRM_DEBUG_DRIVER("drm plane:%d not enabled\n",
> drm_plane->base.id);
> @@ -917,6 +929,9 @@ struct drm_plane *sti_gdp_create(struct drm_device *drm_dev,
>
> sti_gdp_init(gdp);
>
> + /* store the initial value of possible crtcs */
> + gdp->plane.init_possible_crtcs = possible_crtcs;
> +
> res = drm_universal_plane_init(drm_dev, &gdp->plane.drm_plane,
> possible_crtcs,
> &sti_gdp_plane_helpers_funcs,
> diff --git a/drivers/gpu/drm/sti/sti_plane.h b/drivers/gpu/drm/sti/sti_plane.h
> index ce3e8d6..70c5312 100644
> --- a/drivers/gpu/drm/sti/sti_plane.h
> +++ b/drivers/gpu/drm/sti/sti_plane.h
> @@ -66,12 +66,14 @@ struct sti_fps_info {
> * @plane: drm plane it is bound to (if any)
> * @desc: plane type & id
> * @status: to know the status of the plane
> + * @init_possile_crtcs: store the initial possible crtc value
> * @fps_info: frame per second info
> */
> struct sti_plane {
> struct drm_plane drm_plane;
> enum sti_plane_desc desc;
> enum sti_plane_status status;
> + u32 init_possible_crtcs;
> struct sti_fps_info fps_info;
> };
>
> --
> 1.9.1
>
--
Benjamin Gaignard
Graphic Study Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sti: forbid plane on several mixer
2016-09-14 11:40 [PATCH] drm/sti: forbid plane on several mixer Vincent Abriou
2016-09-15 13:06 ` Benjamin Gaignard
@ 2016-09-15 14:27 ` Ville Syrjälä
2016-09-15 14:59 ` Vincent ABRIOU
1 sibling, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2016-09-15 14:27 UTC (permalink / raw)
To: Vincent Abriou; +Cc: Fabien Dessenne, dri-devel, kernel
On Wed, Sep 14, 2016 at 01:40:02PM +0200, Vincent Abriou wrote:
> When a plane is going to be enabled we re-evaluate the possible crtcs
> for the associated drm plane. Only the crtc on which the plane should be
> displayed is considered possible until the plane is disabled.
> Indeed STI hardware does not allow to dynamically change
> the plane's crtc/mixer assignment when the plane is in use (gdp is
> running).
>
> Signed-off-by: Vincent Abriou <vincent.abriou@st.com>
> ---
> drivers/gpu/drm/sti/sti_gdp.c | 15 +++++++++++++++
> drivers/gpu/drm/sti/sti_plane.h | 2 ++
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
> index 3fc62c1..f7cd671 100644
> --- a/drivers/gpu/drm/sti/sti_gdp.c
> +++ b/drivers/gpu/drm/sti/sti_gdp.c
> @@ -71,6 +71,9 @@ static struct gdp_format_to_str {
> #define GDP_NODE_NB_BANK 2
> #define GDP_NODE_PER_FIELD 2
>
> +#define MAIN_CRTC_MASK BIT(0)
> +#define AUX_CRTC_MASK BIT(1)
> +
> struct sti_gdp_node {
> u32 gam_gdp_ctl;
> u32 gam_gdp_agc;
> @@ -690,6 +693,12 @@ static int sti_gdp_atomic_check(struct drm_plane *drm_plane,
> }
> }
>
> + /* re-evaluate the possible crtcs */
> + if (mixer->id == STI_MIXER_MAIN)
> + drm_plane->possible_crtcs = MAIN_CRTC_MASK;
> + else
> + drm_plane->possible_crtcs = AUX_CRTC_MASK;
This stuff isn't meant to be changed dynamically. There's no event for
telling userspace to re-examine this sort of information.
> +
> DRM_DEBUG_KMS("CRTC:%d (%s) drm plane:%d (%s)\n",
> crtc->base.id, sti_mixer_to_str(mixer),
> drm_plane->base.id, sti_plane_to_str(plane));
> @@ -846,6 +855,9 @@ static void sti_gdp_atomic_disable(struct drm_plane *drm_plane,
> {
> struct sti_plane *plane = to_sti_plane(drm_plane);
>
> + /* restore possible crtcs value with the initial value */
> + drm_plane->possible_crtcs = plane->init_possible_crtcs;
> +
> if (!drm_plane->crtc) {
> DRM_DEBUG_DRIVER("drm plane:%d not enabled\n",
> drm_plane->base.id);
> @@ -917,6 +929,9 @@ struct drm_plane *sti_gdp_create(struct drm_device *drm_dev,
>
> sti_gdp_init(gdp);
>
> + /* store the initial value of possible crtcs */
> + gdp->plane.init_possible_crtcs = possible_crtcs;
> +
> res = drm_universal_plane_init(drm_dev, &gdp->plane.drm_plane,
> possible_crtcs,
> &sti_gdp_plane_helpers_funcs,
> diff --git a/drivers/gpu/drm/sti/sti_plane.h b/drivers/gpu/drm/sti/sti_plane.h
> index ce3e8d6..70c5312 100644
> --- a/drivers/gpu/drm/sti/sti_plane.h
> +++ b/drivers/gpu/drm/sti/sti_plane.h
> @@ -66,12 +66,14 @@ struct sti_fps_info {
> * @plane: drm plane it is bound to (if any)
> * @desc: plane type & id
> * @status: to know the status of the plane
> + * @init_possile_crtcs: store the initial possible crtc value
> * @fps_info: frame per second info
> */
> struct sti_plane {
> struct drm_plane drm_plane;
> enum sti_plane_desc desc;
> enum sti_plane_status status;
> + u32 init_possible_crtcs;
> struct sti_fps_info fps_info;
> };
>
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sti: forbid plane on several mixer
2016-09-15 14:27 ` Ville Syrjälä
@ 2016-09-15 14:59 ` Vincent ABRIOU
2016-09-15 15:57 ` Ville Syrjälä
0 siblings, 1 reply; 6+ messages in thread
From: Vincent ABRIOU @ 2016-09-15 14:59 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Fabien DESSENNE, dri-devel@lists.freedesktop.org,
kernel@stlinux.com
On 09/15/2016 04:27 PM, Ville Syrjälä wrote:
> On Wed, Sep 14, 2016 at 01:40:02PM +0200, Vincent Abriou wrote:
>> When a plane is going to be enabled we re-evaluate the possible crtcs
>> for the associated drm plane. Only the crtc on which the plane should be
>> displayed is considered possible until the plane is disabled.
>> Indeed STI hardware does not allow to dynamically change
>> the plane's crtc/mixer assignment when the plane is in use (gdp is
>> running).
>>
>> Signed-off-by: Vincent Abriou <vincent.abriou@st.com>
>> ---
>> drivers/gpu/drm/sti/sti_gdp.c | 15 +++++++++++++++
>> drivers/gpu/drm/sti/sti_plane.h | 2 ++
>> 2 files changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
>> index 3fc62c1..f7cd671 100644
>> --- a/drivers/gpu/drm/sti/sti_gdp.c
>> +++ b/drivers/gpu/drm/sti/sti_gdp.c
>> @@ -71,6 +71,9 @@ static struct gdp_format_to_str {
>> #define GDP_NODE_NB_BANK 2
>> #define GDP_NODE_PER_FIELD 2
>>
>> +#define MAIN_CRTC_MASK BIT(0)
>> +#define AUX_CRTC_MASK BIT(1)
>> +
>> struct sti_gdp_node {
>> u32 gam_gdp_ctl;
>> u32 gam_gdp_agc;
>> @@ -690,6 +693,12 @@ static int sti_gdp_atomic_check(struct drm_plane *drm_plane,
>> }
>> }
>>
>> + /* re-evaluate the possible crtcs */
>> + if (mixer->id == STI_MIXER_MAIN)
>> + drm_plane->possible_crtcs = MAIN_CRTC_MASK;
>> + else
>> + drm_plane->possible_crtcs = AUX_CRTC_MASK;
>
> This stuff isn't meant to be changed dynamically. There's no event for
> telling userspace to re-examine this sort of information.
Yes sure. But by doing this, I let the userspace the ability to fix plan
assignment by it self by re-evaluating the possible CRTC. Before new
plane assignment.
The kernel driver is then flexible enough to avoid Kernel crash.
BR
Vincent
>
>> +
>> DRM_DEBUG_KMS("CRTC:%d (%s) drm plane:%d (%s)\n",
>> crtc->base.id, sti_mixer_to_str(mixer),
>> drm_plane->base.id, sti_plane_to_str(plane));
>> @@ -846,6 +855,9 @@ static void sti_gdp_atomic_disable(struct drm_plane *drm_plane,
>> {
>> struct sti_plane *plane = to_sti_plane(drm_plane);
>>
>> + /* restore possible crtcs value with the initial value */
>> + drm_plane->possible_crtcs = plane->init_possible_crtcs;
>> +
>> if (!drm_plane->crtc) {
>> DRM_DEBUG_DRIVER("drm plane:%d not enabled\n",
>> drm_plane->base.id);
>> @@ -917,6 +929,9 @@ struct drm_plane *sti_gdp_create(struct drm_device *drm_dev,
>>
>> sti_gdp_init(gdp);
>>
>> + /* store the initial value of possible crtcs */
>> + gdp->plane.init_possible_crtcs = possible_crtcs;
>> +
>> res = drm_universal_plane_init(drm_dev, &gdp->plane.drm_plane,
>> possible_crtcs,
>> &sti_gdp_plane_helpers_funcs,
>> diff --git a/drivers/gpu/drm/sti/sti_plane.h b/drivers/gpu/drm/sti/sti_plane.h
>> index ce3e8d6..70c5312 100644
>> --- a/drivers/gpu/drm/sti/sti_plane.h
>> +++ b/drivers/gpu/drm/sti/sti_plane.h
>> @@ -66,12 +66,14 @@ struct sti_fps_info {
>> * @plane: drm plane it is bound to (if any)
>> * @desc: plane type & id
>> * @status: to know the status of the plane
>> + * @init_possile_crtcs: store the initial possible crtc value
>> * @fps_info: frame per second info
>> */
>> struct sti_plane {
>> struct drm_plane drm_plane;
>> enum sti_plane_desc desc;
>> enum sti_plane_status status;
>> + u32 init_possible_crtcs;
>> struct sti_fps_info fps_info;
>> };
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sti: forbid plane on several mixer
2016-09-15 14:59 ` Vincent ABRIOU
@ 2016-09-15 15:57 ` Ville Syrjälä
2016-09-16 9:29 ` Vincent ABRIOU
0 siblings, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2016-09-15 15:57 UTC (permalink / raw)
To: Vincent ABRIOU
Cc: Fabien DESSENNE, dri-devel@lists.freedesktop.org,
kernel@stlinux.com
On Thu, Sep 15, 2016 at 04:59:55PM +0200, Vincent ABRIOU wrote:
>
>
> On 09/15/2016 04:27 PM, Ville Syrjälä wrote:
> > On Wed, Sep 14, 2016 at 01:40:02PM +0200, Vincent Abriou wrote:
> >> When a plane is going to be enabled we re-evaluate the possible crtcs
> >> for the associated drm plane. Only the crtc on which the plane should be
> >> displayed is considered possible until the plane is disabled.
> >> Indeed STI hardware does not allow to dynamically change
> >> the plane's crtc/mixer assignment when the plane is in use (gdp is
> >> running).
> >>
> >> Signed-off-by: Vincent Abriou <vincent.abriou@st.com>
> >> ---
> >> drivers/gpu/drm/sti/sti_gdp.c | 15 +++++++++++++++
> >> drivers/gpu/drm/sti/sti_plane.h | 2 ++
> >> 2 files changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
> >> index 3fc62c1..f7cd671 100644
> >> --- a/drivers/gpu/drm/sti/sti_gdp.c
> >> +++ b/drivers/gpu/drm/sti/sti_gdp.c
> >> @@ -71,6 +71,9 @@ static struct gdp_format_to_str {
> >> #define GDP_NODE_NB_BANK 2
> >> #define GDP_NODE_PER_FIELD 2
> >>
> >> +#define MAIN_CRTC_MASK BIT(0)
> >> +#define AUX_CRTC_MASK BIT(1)
> >> +
> >> struct sti_gdp_node {
> >> u32 gam_gdp_ctl;
> >> u32 gam_gdp_agc;
> >> @@ -690,6 +693,12 @@ static int sti_gdp_atomic_check(struct drm_plane *drm_plane,
> >> }
> >> }
> >>
> >> + /* re-evaluate the possible crtcs */
> >> + if (mixer->id == STI_MIXER_MAIN)
> >> + drm_plane->possible_crtcs = MAIN_CRTC_MASK;
> >> + else
> >> + drm_plane->possible_crtcs = AUX_CRTC_MASK;
> >
> > This stuff isn't meant to be changed dynamically. There's no event for
> > telling userspace to re-examine this sort of information.
>
> Yes sure. But by doing this, I let the userspace the ability to fix plan
> assignment by it self by re-evaluating the possible CRTC. Before new
> plane assignment.
Only if it would re-fetch all planes/crtcs/etc. resources between every
operation. Doing that would suck big time. And with atomic that's not
even a theoretical option since everything would be configured with a
single ioctl.
> The kernel driver is then flexible enough to avoid Kernel crash.
If the kernel crashes due to an an unsupported plane configuration,
then the kernel has to be fixed.
>
> BR
> Vincent
>
> >
> >> +
> >> DRM_DEBUG_KMS("CRTC:%d (%s) drm plane:%d (%s)\n",
> >> crtc->base.id, sti_mixer_to_str(mixer),
> >> drm_plane->base.id, sti_plane_to_str(plane));
> >> @@ -846,6 +855,9 @@ static void sti_gdp_atomic_disable(struct drm_plane *drm_plane,
> >> {
> >> struct sti_plane *plane = to_sti_plane(drm_plane);
> >>
> >> + /* restore possible crtcs value with the initial value */
> >> + drm_plane->possible_crtcs = plane->init_possible_crtcs;
> >> +
> >> if (!drm_plane->crtc) {
> >> DRM_DEBUG_DRIVER("drm plane:%d not enabled\n",
> >> drm_plane->base.id);
> >> @@ -917,6 +929,9 @@ struct drm_plane *sti_gdp_create(struct drm_device *drm_dev,
> >>
> >> sti_gdp_init(gdp);
> >>
> >> + /* store the initial value of possible crtcs */
> >> + gdp->plane.init_possible_crtcs = possible_crtcs;
> >> +
> >> res = drm_universal_plane_init(drm_dev, &gdp->plane.drm_plane,
> >> possible_crtcs,
> >> &sti_gdp_plane_helpers_funcs,
> >> diff --git a/drivers/gpu/drm/sti/sti_plane.h b/drivers/gpu/drm/sti/sti_plane.h
> >> index ce3e8d6..70c5312 100644
> >> --- a/drivers/gpu/drm/sti/sti_plane.h
> >> +++ b/drivers/gpu/drm/sti/sti_plane.h
> >> @@ -66,12 +66,14 @@ struct sti_fps_info {
> >> * @plane: drm plane it is bound to (if any)
> >> * @desc: plane type & id
> >> * @status: to know the status of the plane
> >> + * @init_possile_crtcs: store the initial possible crtc value
> >> * @fps_info: frame per second info
> >> */
> >> struct sti_plane {
> >> struct drm_plane drm_plane;
> >> enum sti_plane_desc desc;
> >> enum sti_plane_status status;
> >> + u32 init_possible_crtcs;
> >> struct sti_fps_info fps_info;
> >> };
> >>
> >> --
> >> 1.9.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/sti: forbid plane on several mixer
2016-09-15 15:57 ` Ville Syrjälä
@ 2016-09-16 9:29 ` Vincent ABRIOU
0 siblings, 0 replies; 6+ messages in thread
From: Vincent ABRIOU @ 2016-09-16 9:29 UTC (permalink / raw)
To: Ville Syrjälä
Cc: dri-devel@lists.freedesktop.org, Fabien DESSENNE,
kernel@stlinux.com
On 09/15/2016 05:57 PM, Ville Syrjälä wrote:
> On Thu, Sep 15, 2016 at 04:59:55PM +0200, Vincent ABRIOU wrote:
>>
>>
>> On 09/15/2016 04:27 PM, Ville Syrjälä wrote:
>>> On Wed, Sep 14, 2016 at 01:40:02PM +0200, Vincent Abriou wrote:
>>>> When a plane is going to be enabled we re-evaluate the possible crtcs
>>>> for the associated drm plane. Only the crtc on which the plane should be
>>>> displayed is considered possible until the plane is disabled.
>>>> Indeed STI hardware does not allow to dynamically change
>>>> the plane's crtc/mixer assignment when the plane is in use (gdp is
>>>> running).
>>>>
>>>> Signed-off-by: Vincent Abriou <vincent.abriou@st.com>
>>>> ---
>>>> drivers/gpu/drm/sti/sti_gdp.c | 15 +++++++++++++++
>>>> drivers/gpu/drm/sti/sti_plane.h | 2 ++
>>>> 2 files changed, 17 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
>>>> index 3fc62c1..f7cd671 100644
>>>> --- a/drivers/gpu/drm/sti/sti_gdp.c
>>>> +++ b/drivers/gpu/drm/sti/sti_gdp.c
>>>> @@ -71,6 +71,9 @@ static struct gdp_format_to_str {
>>>> #define GDP_NODE_NB_BANK 2
>>>> #define GDP_NODE_PER_FIELD 2
>>>>
>>>> +#define MAIN_CRTC_MASK BIT(0)
>>>> +#define AUX_CRTC_MASK BIT(1)
>>>> +
>>>> struct sti_gdp_node {
>>>> u32 gam_gdp_ctl;
>>>> u32 gam_gdp_agc;
>>>> @@ -690,6 +693,12 @@ static int sti_gdp_atomic_check(struct drm_plane *drm_plane,
>>>> }
>>>> }
>>>>
>>>> + /* re-evaluate the possible crtcs */
>>>> + if (mixer->id == STI_MIXER_MAIN)
>>>> + drm_plane->possible_crtcs = MAIN_CRTC_MASK;
>>>> + else
>>>> + drm_plane->possible_crtcs = AUX_CRTC_MASK;
>>>
>>> This stuff isn't meant to be changed dynamically. There's no event for
>>> telling userspace to re-examine this sort of information.
>>
>> Yes sure. But by doing this, I let the userspace the ability to fix plan
>> assignment by it self by re-evaluating the possible CRTC. Before new
>> plane assignment.
>
> Only if it would re-fetch all planes/crtcs/etc. resources between every
> operation. Doing that would suck big time. And with atomic that's not
> even a theoretical option since everything would be configured with a
> single ioctl.
>
I test with weston-1.11.0 and the behavior changed.
It is now forbidden to set a plane on 2 differents CRTC.
I will not going further on this patch.
BR
Vincent
>> The kernel driver is then flexible enough to avoid Kernel crash.
>
> If the kernel crashes due to an an unsupported plane configuration,
> then the kernel has to be fixed.
>
>>
>> BR
>> Vincent
>>
>>>
>>>> +
>>>> DRM_DEBUG_KMS("CRTC:%d (%s) drm plane:%d (%s)\n",
>>>> crtc->base.id, sti_mixer_to_str(mixer),
>>>> drm_plane->base.id, sti_plane_to_str(plane));
>>>> @@ -846,6 +855,9 @@ static void sti_gdp_atomic_disable(struct drm_plane *drm_plane,
>>>> {
>>>> struct sti_plane *plane = to_sti_plane(drm_plane);
>>>>
>>>> + /* restore possible crtcs value with the initial value */
>>>> + drm_plane->possible_crtcs = plane->init_possible_crtcs;
>>>> +
>>>> if (!drm_plane->crtc) {
>>>> DRM_DEBUG_DRIVER("drm plane:%d not enabled\n",
>>>> drm_plane->base.id);
>>>> @@ -917,6 +929,9 @@ struct drm_plane *sti_gdp_create(struct drm_device *drm_dev,
>>>>
>>>> sti_gdp_init(gdp);
>>>>
>>>> + /* store the initial value of possible crtcs */
>>>> + gdp->plane.init_possible_crtcs = possible_crtcs;
>>>> +
>>>> res = drm_universal_plane_init(drm_dev, &gdp->plane.drm_plane,
>>>> possible_crtcs,
>>>> &sti_gdp_plane_helpers_funcs,
>>>> diff --git a/drivers/gpu/drm/sti/sti_plane.h b/drivers/gpu/drm/sti/sti_plane.h
>>>> index ce3e8d6..70c5312 100644
>>>> --- a/drivers/gpu/drm/sti/sti_plane.h
>>>> +++ b/drivers/gpu/drm/sti/sti_plane.h
>>>> @@ -66,12 +66,14 @@ struct sti_fps_info {
>>>> * @plane: drm plane it is bound to (if any)
>>>> * @desc: plane type & id
>>>> * @status: to know the status of the plane
>>>> + * @init_possile_crtcs: store the initial possible crtc value
>>>> * @fps_info: frame per second info
>>>> */
>>>> struct sti_plane {
>>>> struct drm_plane drm_plane;
>>>> enum sti_plane_desc desc;
>>>> enum sti_plane_status status;
>>>> + u32 init_possible_crtcs;
>>>> struct sti_fps_info fps_info;
>>>> };
>>>>
>>>> --
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-16 9:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-14 11:40 [PATCH] drm/sti: forbid plane on several mixer Vincent Abriou
2016-09-15 13:06 ` Benjamin Gaignard
2016-09-15 14:27 ` Ville Syrjälä
2016-09-15 14:59 ` Vincent ABRIOU
2016-09-15 15:57 ` Ville Syrjälä
2016-09-16 9:29 ` Vincent ABRIOU
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.