Re: [PATCH v2 2/8] drm: Add drm_crtc_mode_valid()

[Jose Abreu]

Hi Daniel,


On 10-05-2017 08:59, Daniel Vetter wrote:
> On Tue, May 09, 2017 at 06:00:09PM +0100, Jose Abreu wrote:
>> Add a new helper to call crtc->mode_valid callback.
>>
>> Suggested-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> 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/drm_crtc.c          | 22 ++++++++++++++++++++++
>>  drivers/gpu/drm/drm_crtc_internal.h |  3 +++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index 5af25ce..07ae705 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -38,6 +38,7 @@
>>  #include <drm/drm_crtc.h>
>>  #include <drm/drm_edid.h>
>>  #include <drm/drm_fourcc.h>
>> +#include <drm/drm_modeset_helper_vtables.h>
>>  #include <drm/drm_modeset_lock.h>
>>  #include <drm/drm_atomic.h>
>>  #include <drm/drm_auth.h>
>> @@ -741,3 +742,24 @@ int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
>>  
>>  	return ret;
>>  }
>> +
>> +/**
>> + * drm_crtc_mode_valid - call crtc->mode_valid callback, if any.
>> + * @crtc: crtc
>> + * @mode: mode to be validated
>> + *
>> + * If no mode_valid callback is available this will return MODE_OK.
>> + *
>> + * Returns: drm_mode_status Enum
>> + */
>> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
>> +					 const struct drm_display_mode *mode)
>> +{
>> +	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> This is clearly a helper func, but you place it into the core and
> EXPORT_SYMBOL it. Imo this should be entirely internal to the helpers,
> perhaps just stuff them all into drm_probe_helpers.c? Header file would be
> drm_crtc_helper_internal.h.

Yeah, at first I was not planning to export it but then I saw
that drm_bridge_mode_fixup() is exported (and is in drm_bridge.c)
so it kind of felt right to place this in drm_crtc.c. Anyway, I
will move them to drm_probe_helpers.c, indeed there is no point
in exporting this.

>
> That also means no need for kernel-doc (only the driver api is formally
> documented) and then these 3 patches are so tiny it's better to squash
> them into the patch that adds their users.

Ok, will remove the docs but I think its better to have a single
patch which adds all the helpers so that I can use the
suggested-by tag. Thanks!

Best regards,
Jose Miguel Abreu

>
> Thanks, Daniel
>> +
>> +	if (!crtc_funcs || !crtc_funcs->mode_valid)
>> +		return MODE_OK;
>> +
>> +	return crtc_funcs->mode_valid(crtc, mode);
>> +}
>> +EXPORT_SYMBOL(drm_crtc_mode_valid);
>> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
>> index d077c54..3800abd 100644
>> --- a/drivers/gpu/drm/drm_crtc_internal.h
>> +++ b/drivers/gpu/drm/drm_crtc_internal.h
>> @@ -45,6 +45,9 @@ int drm_crtc_check_viewport(const struct drm_crtc *crtc,
>>  
>>  struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc);
>>  
>> +enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
>> +					 const struct drm_display_mode *mode);
>> +
>>  /* IOCTLs */
>>  int drm_mode_getcrtc(struct drm_device *dev,
>>  		     void *data, struct drm_file *file_priv);
>> -- 
>> 1.9.1
>>
>>