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

[Jose Abreu]

Hi Laurent,


Sorry for the late reply.


On 12-05-2017 10:57, Laurent Pinchart wrote:
> Hi Jose,
>
> Thank you for the patch.
>
> On Tuesday 09 May 2017 18:00:15 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@xxxxxxxxxxxx>
>> Cc: Carlos Palminha <palminha@xxxxxxxxxxxx>
>> Cc: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> Cc: Dave Airlie <airlied@xxxxxxxx>
>> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
>> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
>> ---
>>  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;
> This isn't anything new introduced by this patch, but shouldn't drivers allow 
> for some margin in clock frequencies ? Surely if the mode requires a 
> 60.000.000 Hz frequency and the hardware can only generate 59.999.999 Hz or 
> 60.000.001 Hz we shouldn't fail. As far as I understand, this is something the 
> mode_fixup() operation is supposed to handle, but the arc driver doesn't 
> implement it.

Its funny you mentioned this because I had exactly the same
discussion with Alexey (arcpgu maintainer) last Friday. Perhaps
we could think about a better way for this.

The main problem is that clock driver does not have all the
available clock values, so some modes will not have a compliant
clock value. Right now we are using clk_round_rate() and checking
if the return value matches the one we supplied, but as you
mentioned for 60vs59.94, for example, the mode will still fail to
commit because the clock will not be the same. We reached to the
conclusion that we could in this function have a max deviation
(which would be determined by max allowed tmds variation in HDMI
spec). What do you think?

Initially my idea was that clock driver should support this
variation and just return the same rate if the clock supported it
(i.e. if the deviation was not that much), but, as mentioned by
Alexey, the clock driver is agnostic of TMDS deviation, its just
a clock driver.

>
>> +	return true;
>> +}
> Can't you inline this in arc_pgu_crtc_mode_valid() as there's a single caller 
> ?

Yeah, I will in next version. This was a leftover from previous
version where atomic_check() and mode_valid() did the same
validation.

Thanks!

Best regards,
Jose Miguel Abreu

>>  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,
>>  };