Re: DRM Client capability for aspect ratio

["Nautiyal, Ankit K" <ankit.k.nautiyal@xxxxxxxxx>]

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@xxxxxxxxx> 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

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel