<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <o:p></o:p> <o:p> </o:p> <o:p> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="ProgId" content="Word.Document"> <meta name="Generator" content="Microsoft Word 15"> <meta name="Originator" content="Microsoft Word 15"> <link rel="File-List" href="file:///C:%5CUsers%5CAKNAUTIY%5CAppData%5CLocal%5CTemp%5Cmsohtmlclip1%5C01%5Cclip_filelist.xml">  <link rel="themeData" href="file:///C:%5CUsers%5CAKNAUTIY%5CAppData%5CLocal%5CTemp%5Cmsohtmlclip1%5C01%5Cclip_themedata.thmx"> <link rel="colorSchemeMapping" href="file:///C:%5CUsers%5CAKNAUTIY%5CAppData%5CLocal%5CTemp%5Cmsohtmlclip1%5C01%5Cclip_colorschememapping.xml">  <style>  </style> </o:p> Hi Daniel,<o:p></o:p> Thanks for pointing out the places which will be impacted if aspect ratio support is added as DRM Client Cap.<o:p></o:p> Thanks also to Daniel Vetter, Ville <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> Syrjala, and Shashank Sharma for suggestions and recommendations in various discussions,<o:p></o:p> <o:p> to identify the places where the aspect-ratio support need to be checked.<o:p></o:p> </o:p><o:p> Below is my plan to address these:<o:p></o:p> To add drmClientCap for aspect ratio:<o:p></o:p> 1. Added a variable aspect_ratio_allowed in drm_file struct in file drivers/gpu/drm/drm_ioctl.c<o:p></o:p> This variable can be set or unset.<o:p></o:p> 2. Added DRM_CLIENT_CAP_ASPECT_RATIO macro in drm.h<o:p></o:p> 3. In set_client_capabilities( ) added case for ASPECT_RATIO: drm_ioctl.c<o:p></o:p> </o:p><o:p> The value 0 means user does not recognize the aspect ratio, the value 1 means user recognizes the aspect ratio.<o:p></o:p> </o:p><o:p></o:p><o:p></o:p><o:p></o:p><o:p> 4. Now (drm_file) file_priv->aspect_ratio_allowed must be checked while setting the aspect ratio.<o:p></o:p> 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. <o:p></o:p> 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.<o:p></o:p><o:p></o:p> 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. </o:p><o:p>  <o:p></o:p> <o:p> </o:p> Places where we need to check for aspect ratio support:<o:p></o:p> 1. While setting modes for a connector in the drm_mode_getconnector( ) in drm_connector.c :<o:p></o:p> a. While adding the modes to be enlisted, in drm_mode_getconnector( ),<o:p></o:p> make a check if the aspect_Ratio is supported. If not supported,<o:p></o:p> the mode is not added to the list. This is, similar with what happens with the 3d stereo modes. <o:p></o:p> 2. When modeset is called from ioctl:<o:p></o:p> a. Atomic : drm_atomic.c:<o:p></o:p> In Drm_mode_atomic_ioctl(), blob is passed by the userspace, which has the user mode structure. <o:p></o:p> This blob gets passed to drm_set_atomic_property():<o:p></o:p> i. If user has not asked for aspect ratio (not setting drm client cap for aspect ratio):<o:p></o:p> Whatever flag bits for aspect-ratio are set in user mode structure, just ignore them.<o:p></o:p> In the kernel mode structure, Set the picture_aspect_ratio flag to NONE.<o:p></o:p> Along with this, the blob which is copied into the kernel mode structure (mode_blob)<o:p></o:p> needs to be modified, to have the aspect-ratio flag bits set to none.<o:p></o:p> <o:p></o:p> This is important because, when user calls the drm_get_atomic_property for MODE_ID,<o:p></o:p> it receives the blob from kernel mode structure. Therefore, if no aspect-ratio support,<o:p></o:p> the flags bits of that mode will be unset.<o:p></o:p> <o:p></o:p> ii. if user has asked for aspect ratio, set picture aspect_ratio of kernel mode structure,<o:p></o:p> according to the flag bits of user mode structure.<o:p></o:p> <o:p></o:p> But how to propagate the info that user has asked for aspect ratio support from<o:p></o:p> drm_mode_atomic_ioctl() to subsequent functions where the kernel mode structure<o:p></o:p> is prepared and set (drm_set_atomic_property), which do not have access to file_priv? <o:p></o:p> <o:p></o:p> iii. To achieve this, a new bit field is added in drm_atomic_state structure, aspect_ratio_allowed.<o:p></o:p> This bit is set, in the drm_mode_atomic_ioctl, which has file_priv. Since drm_atomic_state is<o:p></o:p> passed as object to the subsequent functions for doing the modeset, the newly added bit-field<o:p></o:p> aspect_ratio_allowed in drm_atomic_state can be used.<o:p></o:p> <o:p></o:p> a. Legacy : drm_crtc.c :<o:p></o:p> Set the picture_aspect_ratio if the file_priv->aspect_ratio_allowed. <o:p></o:p> <o:p></o:p> 3. When the drm_mode_get_crtc( ) is called in drm_crtc.c:<o:p></o:p> While converting the drm_mode to the drm_mode_mode_info, if aspect_ratio is supported,<o:p></o:p> only then set the aspect_ratio flags of the drm_mode_mode_info.<o:p></o:p> <o:p></o:p> 4. Setting mode for crtc in drm_atomic_helper.c:<o:p></o:p> a. In set_mode_for_crtc() check if bit field aspect_ratio_allowed, is set for the drm_atomic_state. </o:p><o:p><o:p></o:p></o:p> <o:p></o:p> I am currently testing this and am open to suggestions and feedback on this approach. Regards, Ankit <o:p></o:p> <div class="moz-cite-prefix">On 10/19/2017 9:17 PM, Daniel Stone wrote: </div> <blockquote cite="mid:CAPj87rOkpmdFzdpDMOJVGdNxKB8DV_0NsBA8j6ic9OZTbWXOkg@mail.gmail.com" type="cite"> <pre wrap="">Hi Ankit, On 17 October 2017 at 12:08, Nautiyal, Ankit K <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> wrote: </pre> <blockquote type="cite"> <pre wrap="">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, </pre> </blockquote> <pre wrap="">Quick bikeshed suggestion: aspect_ratio_supported. ;) </pre> <blockquote type="cite"> <pre wrap="">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 </pre> </blockquote> <pre wrap="">This would be called in, e.g., drmModeGetCrtc. </pre> <blockquote type="cite"> <pre wrap="">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) </pre> </blockquote> <pre wrap="">As in drmModeSetCrtc, or when doing an atomic modeset, pulling the mode from the blob identified by MODE_ID. </pre> <blockquote type="cite"> <pre wrap="">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. </pre> </blockquote> <pre wrap="">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 </pre> </blockquote> </body> </html>