Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1641407 > unrolled thread

Re: [PATCH v3 6/6] drm: arc: Use crtc->mode_valid() callback

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 6/6] drm: arc: Use crtc->mode_valid() callback Daniel Vetter <daniel@ffwll.ch> - 2017-05-15 11:00 +0200
    Re: [PATCH v3 6/6] drm: arc: Use crtc->mode_valid() callback Daniel Vetter <daniel@ffwll.ch> - 2017-05-15 18:00 +0200
      Re: [PATCH v3 6/6] drm: arc: Use crtc->mode_valid() callback Jose Abreu <Jose.Abreu@synopsys.com> - 2017-05-16 02:00 +0200

#1641407 — Re: [PATCH v3 6/6] drm: arc: Use crtc->mode_valid() callback

On Thu, May 11, 2017 at 10:06:02AM +0100, Jose Abreu wrote:
> Now that we have a callback to check if crtc supports a given mode
> we can use it in arcpgu so that we restrict the number of probbed
> modes to the ones we can actually display.
> 
> This is specially useful because arcpgu crtc is responsible to set
> a clock value in the commit() stage but unfortunatelly this clock
> does not support all the needed ranges.
> 
> Also, remove the atomic_check() callback as mode_valid() callback
> will be called before.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Carlos Palminha <palminha@synopsys.com>
> Cc: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@linux.ie>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Archit Taneja <architt@codeaurora.org>

Btw for justifying your patch series a bit more, would be real sweet to
roll ->mode_valid out to more drivers. I see two easy cases:

bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a
mode_valid callback

bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we
could move the connector_mode valid to the bridge and done.

Care to type these 2 patches on top, just to make this a bit more useful
and a clearer case for merging?

Thanks, Daniel

> ---
>  drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
> index ad9a959..01cae0a 100644
> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> @@ -32,6 +32,18 @@
>  	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
>  };
>  
> +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
> +				  const struct drm_display_mode *mode)
> +{
> +	long rate, clk_rate = mode->clock * 1000;
> +
> +	rate = clk_round_rate(arcpgu->clk, clk_rate);
> +	if (rate != clk_rate)
> +		return false;
> +
> +	return true;
> +}
> +
>  static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
>  {
>  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>  };
>  
> +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
> +					     const struct drm_display_mode *mode)
> +{
> +	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> +
> +	if (!arc_pgu_is_mode_valid(arcpgu, mode))
> +		return MODE_NOCLOCK;
> +
> +	return MODE_OK;
> +}
> +
>  static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
>  {
>  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> @@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
>  			      ~ARCPGU_CTRL_ENABLE_MASK);
>  }
>  
> -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> -				     struct drm_crtc_state *state)
> -{
> -	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> -	struct drm_display_mode *mode = &state->adjusted_mode;
> -	long rate, clk_rate = mode->clock * 1000;
> -
> -	rate = clk_round_rate(arcpgu->clk, clk_rate);
> -	if (rate != clk_rate)
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
>  				      struct drm_crtc_state *state)
>  {
> @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
> +	.mode_valid	= arc_pgu_crtc_mode_valid,
>  	.mode_set	= drm_helper_crtc_mode_set,
>  	.mode_set_base	= drm_helper_crtc_mode_set_base,
>  	.mode_set_nofb	= arc_pgu_crtc_mode_set_nofb,
> @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
>  	.disable	= arc_pgu_crtc_disable,
>  	.prepare	= arc_pgu_crtc_disable,
>  	.commit		= arc_pgu_crtc_enable,
> -	.atomic_check	= arc_pgu_crtc_atomic_check,
>  	.atomic_begin	= arc_pgu_crtc_atomic_begin,
>  };
>  
> -- 
> 1.9.1
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [next] | [standalone]

#1641848

On Mon, May 15, 2017 at 10:53:25AM +0200, Daniel Vetter wrote:
> On Thu, May 11, 2017 at 10:06:02AM +0100, Jose Abreu wrote:
> > Now that we have a callback to check if crtc supports a given mode
> > we can use it in arcpgu so that we restrict the number of probbed
> > modes to the ones we can actually display.
> > 
> > This is specially useful because arcpgu crtc is responsible to set
> > a clock value in the commit() stage but unfortunatelly this clock
> > does not support all the needed ranges.
> > 
> > Also, remove the atomic_check() callback as mode_valid() callback
> > will be called before.
> > 
> > Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> > Cc: Carlos Palminha <palminha@synopsys.com>
> > Cc: Alexey Brodkin <abrodkin@synopsys.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Dave Airlie <airlied@linux.ie>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Archit Taneja <architt@codeaurora.org>
> 
> Btw for justifying your patch series a bit more, would be real sweet to
> roll ->mode_valid out to more drivers. I see two easy cases:
> 
> bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a
> mode_valid callback
> 
> bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we
> could move the connector_mode valid to the bridge and done.
> 
> Care to type these 2 patches on top, just to make this a bit more useful
> and a clearer case for merging?

Aside: There's a pile more drivers (encoders and crtc drivers) which would
similarly benefit and dont really look like they'd be hard to patch up:
- malidp_crtc_mode_fixup
- armada_drm_crtc_mode_fixup (armada isn't atomic, so forget this one)
- atmel_hlcdc_crtc_mode_fixup
- hdmi_mode_fixup in exynos_hdmi.c
- vc4_crtc_mode_fixup and vc4_dpi_encoder_mode_fixup (they both just check
  for interlaced modes, we could probably dump the same check in
  connector->mode_valid).

Plus the 2 bridge drivers. Everyone else does either something more
complex, or something that's not quite correct.
-Daniel

> 
> Thanks, Daniel
> 
> > ---
> >  drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++---------------
> >  1 file changed, 24 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
> > index ad9a959..01cae0a 100644
> > --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
> > +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
> > @@ -32,6 +32,18 @@
> >  	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
> >  };
> >  
> > +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
> > +				  const struct drm_display_mode *mode)
> > +{
> > +	long rate, clk_rate = mode->clock * 1000;
> > +
> > +	rate = clk_round_rate(arcpgu->clk, clk_rate);
> > +	if (rate != clk_rate)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
> >  {
> >  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> > @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
> >  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> >  };
> >  
> > +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
> > +					     const struct drm_display_mode *mode)
> > +{
> > +	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> > +
> > +	if (!arc_pgu_is_mode_valid(arcpgu, mode))
> > +		return MODE_NOCLOCK;
> > +
> > +	return MODE_OK;
> > +}
> > +
> >  static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
> >  {
> >  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> > @@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
> >  			      ~ARCPGU_CTRL_ENABLE_MASK);
> >  }
> >  
> > -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
> > -				     struct drm_crtc_state *state)
> > -{
> > -	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
> > -	struct drm_display_mode *mode = &state->adjusted_mode;
> > -	long rate, clk_rate = mode->clock * 1000;
> > -
> > -	rate = clk_round_rate(arcpgu->clk, clk_rate);
> > -	if (rate != clk_rate)
> > -		return -EINVAL;
> > -
> > -	return 0;
> > -}
> > -
> >  static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
> >  				      struct drm_crtc_state *state)
> >  {
> > @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
> >  }
> >  
> >  static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
> > +	.mode_valid	= arc_pgu_crtc_mode_valid,
> >  	.mode_set	= drm_helper_crtc_mode_set,
> >  	.mode_set_base	= drm_helper_crtc_mode_set_base,
> >  	.mode_set_nofb	= arc_pgu_crtc_mode_set_nofb,
> > @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
> >  	.disable	= arc_pgu_crtc_disable,
> >  	.prepare	= arc_pgu_crtc_disable,
> >  	.commit		= arc_pgu_crtc_enable,
> > -	.atomic_check	= arc_pgu_crtc_atomic_check,
> >  	.atomic_begin	= arc_pgu_crtc_atomic_begin,
> >  };
> >  
> > -- 
> > 1.9.1
> > 
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1642132

Hi Daniel,


On 15-05-2017 16:52, Daniel Vetter wrote:
> On Mon, May 15, 2017 at 10:53:25AM +0200, Daniel Vetter wrote:
>> On Thu, May 11, 2017 at 10:06:02AM +0100, Jose Abreu wrote:
>>> Now that we have a callback to check if crtc supports a given mode
>>> we can use it in arcpgu so that we restrict the number of probbed
>>> modes to the ones we can actually display.
>>>
>>> This is specially useful because arcpgu crtc is responsible to set
>>> a clock value in the commit() stage but unfortunatelly this clock
>>> does not support all the needed ranges.
>>>
>>> Also, remove the atomic_check() callback as mode_valid() callback
>>> will be called before.
>>>
>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Dave Airlie <airlied@linux.ie>
>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>>> Cc: Archit Taneja <architt@codeaurora.org>
>> Btw for justifying your patch series a bit more, would be real sweet to
>> roll ->mode_valid out to more drivers. I see two easy cases:
>>
>> bridge/analogix-anx78xx.c: has a simple mode_fixup which really is just a
>> mode_valid callback
>>
>> bridge/synopsys/dw-hdmi.c: simply hand-rolls what you've done here, we
>> could move the connector_mode valid to the bridge and done.
>>
>> Care to type these 2 patches on top, just to make this a bit more useful
>> and a clearer case for merging?
> Aside: There's a pile more drivers (encoders and crtc drivers) which would
> similarly benefit and dont really look like they'd be hard to patch up:
> - malidp_crtc_mode_fixup
> - armada_drm_crtc_mode_fixup (armada isn't atomic, so forget this one)
> - atmel_hlcdc_crtc_mode_fixup
> - hdmi_mode_fixup in exynos_hdmi.c
> - vc4_crtc_mode_fixup and vc4_dpi_encoder_mode_fixup (they both just check
>   for interlaced modes, we could probably dump the same check in
>   connector->mode_valid).
>
> Plus the 2 bridge drivers. Everyone else does either something more
> complex, or something that's not quite correct.
> -Daniel

Sorry for being silent but I am away from office this week. I
will try to evaluate and add this in next version. In the
meantime I will review the documentation patches you sent. Thanks!

Best regards,
Jose Miguel Abreu

>
>> Thanks, Daniel
>>
>>> ---
>>>  drivers/gpu/drm/arc/arcpgu_crtc.c | 39 ++++++++++++++++++++++++---------------
>>>  1 file changed, 24 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c
>>> index ad9a959..01cae0a 100644
>>> --- a/drivers/gpu/drm/arc/arcpgu_crtc.c
>>> +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c
>>> @@ -32,6 +32,18 @@
>>>  	{ "r8g8b8", 24, {16, 8}, {8, 8}, {0, 8}, {0, 0}, DRM_FORMAT_RGB888 },
>>>  };
>>>  
>>> +static bool arc_pgu_is_mode_valid(struct arcpgu_drm_private *arcpgu,
>>> +				  const struct drm_display_mode *mode)
>>> +{
>>> +	long rate, clk_rate = mode->clock * 1000;
>>> +
>>> +	rate = clk_round_rate(arcpgu->clk, clk_rate);
>>> +	if (rate != clk_rate)
>>> +		return false;
>>> +
>>> +	return true;
>>> +}
>>> +
>>>  static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
>>>  {
>>>  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>>> @@ -64,6 +76,17 @@ static void arc_pgu_set_pxl_fmt(struct drm_crtc *crtc)
>>>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>>  };
>>>  
>>> +enum drm_mode_status arc_pgu_crtc_mode_valid(struct drm_crtc *crtc,
>>> +					     const struct drm_display_mode *mode)
>>> +{
>>> +	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>>> +
>>> +	if (!arc_pgu_is_mode_valid(arcpgu, mode))
>>> +		return MODE_NOCLOCK;
>>> +
>>> +	return MODE_OK;
>>> +}
>>> +
>>>  static void arc_pgu_crtc_mode_set_nofb(struct drm_crtc *crtc)
>>>  {
>>>  	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>>> @@ -129,20 +152,6 @@ static void arc_pgu_crtc_disable(struct drm_crtc *crtc)
>>>  			      ~ARCPGU_CTRL_ENABLE_MASK);
>>>  }
>>>  
>>> -static int arc_pgu_crtc_atomic_check(struct drm_crtc *crtc,
>>> -				     struct drm_crtc_state *state)
>>> -{
>>> -	struct arcpgu_drm_private *arcpgu = crtc_to_arcpgu_priv(crtc);
>>> -	struct drm_display_mode *mode = &state->adjusted_mode;
>>> -	long rate, clk_rate = mode->clock * 1000;
>>> -
>>> -	rate = clk_round_rate(arcpgu->clk, clk_rate);
>>> -	if (rate != clk_rate)
>>> -		return -EINVAL;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>>  static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
>>>  				      struct drm_crtc_state *state)
>>>  {
>>> @@ -158,6 +167,7 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
>>>  }
>>>  
>>>  static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = {
>>> +	.mode_valid	= arc_pgu_crtc_mode_valid,
>>>  	.mode_set	= drm_helper_crtc_mode_set,
>>>  	.mode_set_base	= drm_helper_crtc_mode_set_base,
>>>  	.mode_set_nofb	= arc_pgu_crtc_mode_set_nofb,
>>> @@ -165,7 +175,6 @@ static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc,
>>>  	.disable	= arc_pgu_crtc_disable,
>>>  	.prepare	= arc_pgu_crtc_disable,
>>>  	.commit		= arc_pgu_crtc_enable,
>>> -	.atomic_check	= arc_pgu_crtc_atomic_check,
>>>  	.atomic_begin	= arc_pgu_crtc_atomic_begin,
>>>  };
>>>  
>>> -- 
>>> 1.9.1
>>>
>>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__blog.ffwll.ch&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=yaVFU4TjGY0gVF8El1uKcisy6TPsyCl9uN7Wsis-qhY&m=v9tZNfdrkm8mrAlJyEpm71Te7tDReAFXijS23BkDSTo&s=Y6DG3RpFLs0nomQ09FA3hqCqq6MQ3dvG81s_2XlVytM&e= 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web