Re: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 03, 2016 at 07:04:37PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/3/2016 6:56 PM, Ville Syrjälä wrote:
> > On Thu, Nov 03, 2016 at 06:40:11PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 11/3/2016 6:32 PM, Ville Syrjälä wrote:
> >>> On Thu, Nov 03, 2016 at 12:47:39PM +0000, Sharma, Shashank wrote:
> >>>> Hi Ville,
> >>>> (-dri-devel)
> >>>> I would appreciate an internal discussion before going to dri-devel. a
> >>> Added back. Private discussions are pointless.
> >> Noted.
> >>>> What did this patch break ?
> >>> Intel ddx. It will reject any mode with flags it doesn't know about.
> >>> Other ddxen like -ati or -modesetting will also suffer, but potentially
> >>> in a different way since they don't even check the flags and instead
> >>> just directly stuff them into the randr mode flags, which will result
> >>> in an out of spec mode as far randr is concerned.
> >> Then its a fault of intel-ddx design. We are adding new flags to
> >> indicate aspect ratios, so its obvious that the implementation should be
> >> modified to accept new flags.
> >> I dont see why you should revert patch for it.
> > Rule 1 of kernel development: You simply don't break existing userspace
> Again, we are adding something new, for which userspace should be modified.
> If the userspace in written in such a way that it wont accept changes, 
> its not saleable design.

Whether the design is good or not is besides the point. It already
exists, so we have to live with it.

> 
> Even in this case, we should go and extend the userspace (which I am 
> ready to do) instead of reverting the patch.

We can't retroactively fix the binaries on everyone's machine.

> If you still think you should send this revert, I am removing my NACK. 
> Pls Go ahead.

The other option is to not revert and instead slap a fix on top. But 
that would have to be done reasonably quickly so that the thing is
ready in time for 4.10. We're closing in on 4.9-rc4 now so I guess
we should still have a few weeks to fix things up. Whether that's
enough I don't know. If not, then we should revert.

> >>>> This is exposed in the same way, as the 3D flags.
> >>> Not it isn't. We have a client cap for 3D flags.
> >>>
> >>>> The information is already available in form of flags, all you have to do in userspace is read and print that. Its already being done in Android.
> >>> Please stop relying on Android for testing upstream patches. Test with
> >>> actual userspace people are using.
> >> Please note that android goes through HDMI compliance certification, and
> >> several commercial test infrastructures, so
> >> that's one of the best infrastructure to test. And when we have common
> >> branch for all of Linux flavors, we should consider all.
> > You can test with Android as much as you like. That probably won't
> > catch ABI breaks and whatnot though since you don't generally just plop
> > a new kernel onto an existing Android device.
> >
> > So you really must test with normal userspace as well. That has a long
> > history and so any ABI/behaviour change is actually a big deal.
> >
> >>>>    
> >>>> NACK until we get to the right reason.
> >>>>
> >>>> Regards
> >>>> Shashank
> >>>> -----Original Message-----
> >>>> From: ville.syrjala@linux.intel.com [mailto:ville.syrjala@linux.intel.com]
> >>>> Sent: Thursday, November 3, 2016 6:02 PM
> >>>> To: dri-devel@lists.freedesktop.org
> >>>> Cc: Sharma, Shashank <shashank.sharma@intel.com>; Lin; Jia, Lin A <lin.a.jia@intel.com>; Sharma, Akashdeep <akashdeep.sharma@intel.com>; Jim Bride <jim.bride@linux.intel.com>; Jose Abreu <Jose.Abreu@synopsys.com>; Daniel Vetter <daniel.vetter@ffwll.ch>; Emil Velikov <emil.l.velikov@gmail.com>
> >>>> Subject: [PATCH 1/2] Revert "drm: Add and handle new aspect ratios in DRM layer"
> >>>>
> >>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>
> >>>> This reverts commit a68362fe3e84fcbedd49939aa200519aa5410135.
> >>>>
> >>>> Adding new mode flags willy nilly breaks existing userspace. We need to coordinate this better, potentially with a new client cap that only exposes the aspect ratio flags when userspace is prepared for them (similar to what we do with stereo 3D modes).
> >>>>
> >>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
> >>>> Cc: Lin, Jia <lin.a.jia@intel.com>
> >>>> Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
> >>>> Cc: Jim Bride <jim.bride@linux.intel.com>
> >>>> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/drm_modes.c | 12 ------------  include/uapi/drm/drm_mode.h |  6 ------
> >>>>    2 files changed, 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index f64ac86deb84..725faa6409aa 100644
> >>>> --- a/drivers/gpu/drm/drm_modes.c
> >>>> +++ b/drivers/gpu/drm/drm_modes.c
> >>>> @@ -1513,12 +1513,6 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> >>>>    	case HDMI_PICTURE_ASPECT_16_9:
> >>>>    		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> >>>>    		break;
> >>>> -	case HDMI_PICTURE_ASPECT_64_27:
> >>>> -		out->flags |= DRM_MODE_FLAG_PIC_AR_64_27;
> >>>> -		break;
> >>>> -	case DRM_MODE_PICTURE_ASPECT_256_135:
> >>>> -		out->flags |= DRM_MODE_FLAG_PIC_AR_256_135;
> >>>> -		break;
> >>>>    	case HDMI_PICTURE_ASPECT_RESERVED:
> >>>>    	default:
> >>>>    		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; @@ -1580,12 +1574,6 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
> >>>>    	case DRM_MODE_FLAG_PIC_AR_16_9:
> >>>>    		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> >>>>    		break;
> >>>> -	case DRM_MODE_FLAG_PIC_AR_64_27:
> >>>> -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27;
> >>>> -		break;
> >>>> -	case DRM_MODE_FLAG_PIC_AR_256_135:
> >>>> -		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135;
> >>>> -		break;
> >>>>    	default:
> >>>>    		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>>>    		break;
> >>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 084b50a02dc5..5c142b1387ac 100644
> >>>> --- a/include/uapi/drm/drm_mode.h
> >>>> +++ b/include/uapi/drm/drm_mode.h
> >>>> @@ -81,8 +81,6 @@ extern "C" {
> >>>>    #define DRM_MODE_PICTURE_ASPECT_NONE		0
> >>>>    #define DRM_MODE_PICTURE_ASPECT_4_3		1
> >>>>    #define DRM_MODE_PICTURE_ASPECT_16_9		2
> >>>> -#define DRM_MODE_PICTURE_ASPECT_64_27		3
> >>>> -#define DRM_MODE_PICTURE_ASPECT_256_135		4
> >>>>    
> >>>>    /* Aspect ratio flag bitmask (4 bits 22:19) */
> >>>>    #define DRM_MODE_FLAG_PIC_AR_MASK		(0x0F<<19)
> >>>> @@ -92,10 +90,6 @@ extern "C" {
> >>>>    			(DRM_MODE_PICTURE_ASPECT_4_3<<19)
> >>>>    #define  DRM_MODE_FLAG_PIC_AR_16_9 \
> >>>>    			(DRM_MODE_PICTURE_ASPECT_16_9<<19)
> >>>> -#define  DRM_MODE_FLAG_PIC_AR_64_27 \
> >>>> -			(DRM_MODE_PICTURE_ASPECT_64_27<<19)
> >>>> -#define  DRM_MODE_FLAG_PIC_AR_256_135 \
> >>>> -			(DRM_MODE_PICTURE_ASPECT_256_135<<19)
> >>>>    
> >>>>    /* DPMS flags */
> >>>>    /* bit compatible with the xorg definitions. */
> >>>> --
> >>>> 2.7.4
> >>>>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel