Re: [PATCH 1/2] drm/fb-helper: Make fbdev inherit the crtc's rotation

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 26-04-17 14:13, Bastien Nocera wrote:
> On Sun, 2017-04-23 at 18:11 +0200, Hans de Goede wrote:
>> From: Ville Syrjala <ville.syrjala@linux.intel.com>
>>
>> If a connector added through drm_fb_helper_add_one_connector() has
>> a crtc attached and that crtc has a rotation configured make the
>> fbdev inherit the crtc's rotation.
>>
>> This is useful on e.g. some tablets which have their lcd panel
>> mounted
>> upside down, which before this commit would result in the kernel boot
>> messages switching from being shown the right way up in efifb to
>> being
>> shown upside down as soon as a native kms driver loads.
>>
>> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=94894
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> [hdegoede@redhat.com: Split the drm/fb-helper bits out of Ville's
>>   "drm/fb-helper: Inherit rotation wip" patch]
>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 51
>> ++++++++++++++++++++++++++++++++++++++---
>>   include/drm/drm_fb_helper.h     |  2 ++
>>   2 files changed, 50 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index 324a688..c97e00ab 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -159,6 +159,8 @@ int drm_fb_helper_add_one_connector(struct
>> drm_fb_helper *fb_helper, struct drm_
>>   {
>>   	struct drm_fb_helper_connector **temp;
>>   	struct drm_fb_helper_connector *fb_helper_connector;
>> +	struct drm_crtc *crtc = connector->encoder ?
>> +				connector->encoder->crtc : NULL;
>>   
>>   	if (!drm_fbdev_emulation)
>>   		return 0;
>> @@ -180,6 +182,11 @@ int drm_fb_helper_add_one_connector(struct
>> drm_fb_helper *fb_helper, struct drm_
>>   
>>   	drm_connector_reference(connector);
>>   	fb_helper_connector->connector = connector;
>> +	if (crtc && crtc->primary->state)
>> +		fb_helper_connector->rotation = crtc->primary-
>>> state->rotation;
>> +	if (!fb_helper_connector->rotation)
>> +		fb_helper_connector->rotation = DRM_ROTATE_0;
> 
> That's equivalent to:
> if (fb_helper_connector->rotation = DRM_ROTATE_0)
>     fb_helper_connector->rotation = DRM_ROTATE_0;

No it is not equivalent:

include/drm/drm_blend.h
40:#define DRM_ROTATE_0 BIT(0)

and BIT(0) is (1 << 0)

Regards,

Hans

> 
> Maybe:
> if (crtc && crtc->primary->state)
>     ...
> else
>     ...->rotation = DRM_ROTATE_0;
> would be clearer? Or simply omit it if the connector is zero'ed.
> 
> <snip>
>> diff --git a/include/drm/drm_fb_helper.h
>> b/include/drm/drm_fb_helper.h
>> index 6f5aceb..19fc313 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -48,6 +48,7 @@ struct drm_fb_helper_crtc {
>>   	struct drm_mode_set mode_set;
>>   	struct drm_display_mode *desired_mode;
>>   	int x, y;
>> +	uint8_t rotation;
>>   };
>>   
>>   /**
>> @@ -159,6 +160,7 @@ struct drm_fb_helper_funcs {
>>   
>>   struct drm_fb_helper_connector {
>>   	struct drm_connector *connector;
>> +	uint8_t rotation;
> 
> In both those cases, I'd add a mention of the type of enum/mask etc.
> for the rotation, for example:
> uint8_t rotation; /* DRM_ROTATE_* */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx