Re: [PATCH] drm/sti: forbid plane on several mixer

["Ville Syrjälä" <ville.syrjala@linux.intel.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