DRM Client capability for aspect ratio

[Nautiyal, Ankit K]

Hi Daniel,

Thanks for pointing out the places which will be impacted if aspect 
ratio support is added as DRM Client Cap.

Thanks also to Daniel Vetter, Ville Syrjala, and Shashank Sharma for 
suggestions and recommendations in various discussions,

to identify the places where the aspect-ratio support need to be checked.

Below is my plan to address these:

*    To add drmClientCap for aspect ratio:*

1. Added a variable aspect_ratio_allowed in drm_file struct in file 
drivers/gpu/drm/drm_ioctl.c

This variable can be set or unset.

2. Added DRM_CLIENT_CAP_ASPECT_RATIO macro in drm.h

3. In set_client_capabilities( ) added case for ASPECT_RATIO: drm_ioctl.c

The value 0 means user does not recognize the aspect ratio,the value 1 
means user recognizes the aspect ratio.

4. Now (drm_file) file_priv->aspect_ratio_allowed must be checked while 
setting the aspect ratio.


Problem is that there are places where file_priv is not accessible, how 
to pass aspect ratio supported info to those places.

Passing file_priv to different functions would not be possible and would 
require massive change in function definitions.


To address this the drm_atomic_state is modified to have a bit field 
"aspect_ratio_allowed" this is set,

if file_priv->aspect_ratio_supported.We do this as drm_atomic_state is 
already passed and available directly to these places,

or can be derived from drm_crtc_state which has pointer to drm_atomic_state.


*Places where we need to check for aspect ratio support:*

*1.*While setting modes for a connector in the drm_mode_getconnector( ) 
in drm_connector.c :

a. While adding the modes to be enlisted, in drm_mode_getconnector( ),

         make a check if the aspect_Ratio is supported. If not supported,

         the mode is not added to the list. This is, similar with what 
happens with the 3d stereo modes.

*2. *When modeset is called from ioctl:

a. *Atomic*: drm_atomic.c:

In Drm_mode_atomic_ioctl(), blob is passed by the userspace, which has 
the user mode structure.

This blob gets passed to drm_set_atomic_property():

i.    If user has *not asked* for aspect ratio (not setting drm client 
cap for aspect ratio):

Whatever flag bits for aspect-ratio are set in user mode structure, just 
ignore them.

In the kernel mode structure, Set the picture_aspect_ratio flag to NONE.

Along with this, the blob which is copied into the kernel mode structure 
(mode_blob)

needs to be modified, to have the aspect-ratio flag bits set to none.

This is important because, when user calls the drm_get_atomic_property 
for MODE_ID,

it receives the blob from kernel mode structure. Therefore, if no 
aspect-ratio support,

the flags bits of that mode will be unset.

ii.    if user has asked for aspect ratio, set picture aspect_ratio of 
kernel mode structure,

        according to the flag bits of user mode structure.

But how to propagate the info that user has asked for aspect ratio 
support from

drm_mode_atomic_ioctl() to subsequent functions where the kernel mode 
structure

is prepared and set (drm_set_atomic_property), which do not have access 
to file_priv?

iii. To achieve this, a new bit field is added in drm_atomic_state 
structure, aspect_ratio_allowed.

       This bit is set, in the drm_mode_atomic_ioctl, which has 
file_priv. Since drm_atomic_state is

       passed as object to the subsequent functions for doing the 
modeset, the newly added bit-field

aspect_ratio_allowed in drm_atomic_state can be used.

a. *Legacy*: drm_crtc.c :

     Set the picture_aspect_ratio if the file_priv->aspect_ratio_allowed.

*3.*When the drm_mode_get_crtc( ) is called in drm_crtc.c:

While converting the drm_mode to the drm_mode_mode_info, if aspect_ratio 
is supported,

only then set the aspect_ratio flags of the drm_mode_mode_info.

*4.*Setting mode for crtc in drm_atomic_helper.c:

a. In set_mode_for_crtc() check if bit field aspect_ratio_allowed, is 
set for the drm_atomic_state.


I am currently testing this and am open to suggestions and feedback on 
this approach.


Regards,

Ankit



On 10/19/2017 9:17 PM, Daniel Stone wrote:
> Hi Ankit,
>
> On 17 October 2017 at 12:08, Nautiyal, Ankit K
> <ankit.k.nautiyal at intel.com>  wrote:
>> On Similar lines, I was able to add the new DRM_CLIENT_CAP_ASPECT_RATIO. I
>> also added a member 'aspect_ratio_required' in drm_file structure,
> Quick bikeshed suggestion: aspect_ratio_supported. ;)
>
>> Now what I want to do is
>>
>> 1.     Setting the aspect-ratio flag-bits (19-22) in drmModeModeInfo only if
>> the client advertises that it requires the aspect-ratio.
>>
>> So I need to have this information (that client requires aspect ratio)
>> available in the function:
>>
>> drm_mode_convert_to_umode( ) : Where the flag bits are actually set
> This would be called in, e.g., drmModeGetCrtc.
>
>> 2.     Similarly in case of modeset request from the client, we would want
>> to parse the aspect ratio bits from the requested  drmModeModeInfo only if
>> the client requires aspect ratio.
>>
>> This will require change in the function:
>>
>> drm_mode_convert_umode()  : Where the flag bits of  drm_mode_mode_info are
>> read by the drm layer to determine the aspect ratio and set in
>> drm_display_mode)
> As in drmModeSetCrtc, or when doing an atomic modeset, pulling the
> mode from the blob identified by MODE_ID.
>
>> The problem is that, both these functions have drm_mode_mode_info and
>> drm_display_mode as the only arguments, but our flag aspect_ratio_required
>> is in file_priv (struct drm_file).
>>
>> To do this there are two ways I can think of:
>>
>> 1.     Change the drm_mode_convert_umode() to include aspect_ratio_required
>> flag.
>>
>>          Before calling this function, the file_priv->aspect_ratio_required
>> will be checked and the flag will be set accordingly.
>>
>> 2.     While getting the drmClientCap from client, instead of storing this
>> information in file-priv, store this info in some other structure.
> I think you could just pass the file_priv directly; it seems a bit more clear.
>
> One case I think you're missing though, is where a client discovers an
> existing MODE_ID blob (via the atomic property), then gets the blob
> contents to parse. In this case, we might pass the client unsupported
> flags. I don't have a really good suggestion for fixing this, apart
> from duplicating the blob content and returning a different result to
> userspace ... !
>
> Cheers,
> Daniel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20171031/f589d74c/attachment-0001.html>