diff mbox series

[1/7] drm/tidss: Add CRTC mode_fixup

Message ID 20240511153051.1355825-2-a-bhatia1@ti.com (mailing list archive)
State New, archived
Headers show
Series drm/bridge: cdns-dsi: Fix the color-shift issue | expand

Commit Message

Aradhya Bhatia May 11, 2024, 3:30 p.m. UTC
Add support for mode_fixup for the tidss CRTC.

Some bridges like the cdns-dsi consume the crtc_* timing parameters for
programming the blanking values. Allow for the normal timing parameters
to get copied to crtc_* timing params.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_crtc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Maxime Ripard May 16, 2024, 8:10 a.m. UTC | #1
Hi,

On Sat, May 11, 2024 at 09:00:45PM +0530, Aradhya Bhatia wrote:
> Add support for mode_fixup for the tidss CRTC.
> 
> Some bridges like the cdns-dsi consume the crtc_* timing parameters for
> programming the blanking values. Allow for the normal timing parameters
> to get copied to crtc_* timing params.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  drivers/gpu/drm/tidss/tidss_crtc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 94f8e3178df5..797ef53d9ad2 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -309,12 +309,23 @@ enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
>  	return dispc_vp_mode_valid(tidss->dispc, tcrtc->hw_videoport, mode);
>  }
>  
> +static
> +bool tidss_crtc_mode_fixup(struct drm_crtc *crtc,
> +			   const struct drm_display_mode *mode,
> +			   struct drm_display_mode *adjusted_mode)
> +{
> +	drm_mode_set_crtcinfo(adjusted_mode, 0);
> +
> +	return true;
> +}
> +
>  static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
>  	.atomic_check = tidss_crtc_atomic_check,
>  	.atomic_flush = tidss_crtc_atomic_flush,
>  	.atomic_enable = tidss_crtc_atomic_enable,
>  	.atomic_disable = tidss_crtc_atomic_disable,
>  
> +	.mode_fixup = tidss_crtc_mode_fixup,
>  	.mode_valid = tidss_crtc_mode_valid,
>  };

mode_fixup is deprecated for atomic drivers, so the solution must be
different there.

It's also not clear to me how it could change anything there:
drm_mode_set_crtcinfo with no flags will make crtc_* field exactly
identical to their !crtc counterparts.

Maxime
Aradhya Bhatia May 16, 2024, 11:03 a.m. UTC | #2
Hi Maxime,

Thank you for reviewing the patches.

On 16/05/24 13:40, Maxime Ripard wrote:
> Hi,
> 
> On Sat, May 11, 2024 at 09:00:45PM +0530, Aradhya Bhatia wrote:
>> Add support for mode_fixup for the tidss CRTC.
>>
>> Some bridges like the cdns-dsi consume the crtc_* timing parameters for
>> programming the blanking values. Allow for the normal timing parameters
>> to get copied to crtc_* timing params.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>  drivers/gpu/drm/tidss/tidss_crtc.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
>> index 94f8e3178df5..797ef53d9ad2 100644
>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
>> @@ -309,12 +309,23 @@ enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
>>  	return dispc_vp_mode_valid(tidss->dispc, tcrtc->hw_videoport, mode);
>>  }
>>  
>> +static
>> +bool tidss_crtc_mode_fixup(struct drm_crtc *crtc,
>> +			   const struct drm_display_mode *mode,
>> +			   struct drm_display_mode *adjusted_mode)
>> +{
>> +	drm_mode_set_crtcinfo(adjusted_mode, 0);
>> +
>> +	return true;
>> +}
>> +
>>  static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
>>  	.atomic_check = tidss_crtc_atomic_check,
>>  	.atomic_flush = tidss_crtc_atomic_flush,
>>  	.atomic_enable = tidss_crtc_atomic_enable,
>>  	.atomic_disable = tidss_crtc_atomic_disable,
>>  
>> +	.mode_fixup = tidss_crtc_mode_fixup,
>>  	.mode_valid = tidss_crtc_mode_valid,
>>  };
> 
> mode_fixup is deprecated for atomic drivers, so the solution must be
> different there.
> 
> It's also not clear to me how it could change anything there:
> drm_mode_set_crtcinfo with no flags will make crtc_* field exactly
> identical to their !crtc counterparts.
>

I checked the flag options. There isn't any flag required. The only
reason to add this call is because cdns-dsi strictly requires the crtc_*
fields to be populated during the bridge enable.

Secondly, if mode_fixup is deprecated, I think the crtc_atomic_check
would be the next best place to add this call.


Regards
Aradhya
Maxime Ripard May 21, 2024, 1:18 p.m. UTC | #3
On Thu, May 16, 2024 at 04:33:40PM GMT, Aradhya Bhatia wrote:
> Hi Maxime,
> 
> Thank you for reviewing the patches.
> 
> On 16/05/24 13:40, Maxime Ripard wrote:
> > Hi,
> > 
> > On Sat, May 11, 2024 at 09:00:45PM +0530, Aradhya Bhatia wrote:
> >> Add support for mode_fixup for the tidss CRTC.
> >>
> >> Some bridges like the cdns-dsi consume the crtc_* timing parameters for
> >> programming the blanking values. Allow for the normal timing parameters
> >> to get copied to crtc_* timing params.
> >>
> >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> >> ---
> >>  drivers/gpu/drm/tidss/tidss_crtc.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> >> index 94f8e3178df5..797ef53d9ad2 100644
> >> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> >> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> >> @@ -309,12 +309,23 @@ enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
> >>  	return dispc_vp_mode_valid(tidss->dispc, tcrtc->hw_videoport, mode);
> >>  }
> >>  
> >> +static
> >> +bool tidss_crtc_mode_fixup(struct drm_crtc *crtc,
> >> +			   const struct drm_display_mode *mode,
> >> +			   struct drm_display_mode *adjusted_mode)
> >> +{
> >> +	drm_mode_set_crtcinfo(adjusted_mode, 0);
> >> +
> >> +	return true;
> >> +}
> >> +
> >>  static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
> >>  	.atomic_check = tidss_crtc_atomic_check,
> >>  	.atomic_flush = tidss_crtc_atomic_flush,
> >>  	.atomic_enable = tidss_crtc_atomic_enable,
> >>  	.atomic_disable = tidss_crtc_atomic_disable,
> >>  
> >> +	.mode_fixup = tidss_crtc_mode_fixup,
> >>  	.mode_valid = tidss_crtc_mode_valid,
> >>  };
> > 
> > mode_fixup is deprecated for atomic drivers, so the solution must be
> > different there.
> > 
> > It's also not clear to me how it could change anything there:
> > drm_mode_set_crtcinfo with no flags will make crtc_* field exactly
> > identical to their !crtc counterparts.
> >
> 
> I checked the flag options. There isn't any flag required. The only
> reason to add this call is because cdns-dsi strictly requires the crtc_*
> fields to be populated during the bridge enable.
> 
> Secondly, if mode_fixup is deprecated, I think the crtc_atomic_check
> would be the next best place to add this call.

That would be better, yes, but we shouldn't even have to do that in the
first place. AFAIK all the path that create a drm_display_mode will call
drm_mode_set_crtcinfo on it to fill those fields. So if they are missing
somewhere, that's what the actual bug is, not something we should work
around of at the driver level.

Maxime
Aradhya Bhatia May 30, 2024, 9:39 a.m. UTC | #4
Hi Maxime,

On 21/05/24 18:48, Maxime Ripard wrote:
> On Thu, May 16, 2024 at 04:33:40PM GMT, Aradhya Bhatia wrote:
>> Hi Maxime,
>>
>> Thank you for reviewing the patches.
>>
>> On 16/05/24 13:40, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Sat, May 11, 2024 at 09:00:45PM +0530, Aradhya Bhatia wrote:
>>>> Add support for mode_fixup for the tidss CRTC.
>>>>
>>>> Some bridges like the cdns-dsi consume the crtc_* timing parameters for
>>>> programming the blanking values. Allow for the normal timing parameters
>>>> to get copied to crtc_* timing params.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> ---
>>>>  drivers/gpu/drm/tidss/tidss_crtc.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
>>>> index 94f8e3178df5..797ef53d9ad2 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
>>>> @@ -309,12 +309,23 @@ enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
>>>>  	return dispc_vp_mode_valid(tidss->dispc, tcrtc->hw_videoport, mode);
>>>>  }
>>>>  
>>>> +static
>>>> +bool tidss_crtc_mode_fixup(struct drm_crtc *crtc,
>>>> +			   const struct drm_display_mode *mode,
>>>> +			   struct drm_display_mode *adjusted_mode)
>>>> +{
>>>> +	drm_mode_set_crtcinfo(adjusted_mode, 0);
>>>> +
>>>> +	return true;
>>>> +}
>>>> +
>>>>  static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
>>>>  	.atomic_check = tidss_crtc_atomic_check,
>>>>  	.atomic_flush = tidss_crtc_atomic_flush,
>>>>  	.atomic_enable = tidss_crtc_atomic_enable,
>>>>  	.atomic_disable = tidss_crtc_atomic_disable,
>>>>  
>>>> +	.mode_fixup = tidss_crtc_mode_fixup,
>>>>  	.mode_valid = tidss_crtc_mode_valid,
>>>>  };
>>>
>>> mode_fixup is deprecated for atomic drivers, so the solution must be
>>> different there.
>>>
>>> It's also not clear to me how it could change anything there:
>>> drm_mode_set_crtcinfo with no flags will make crtc_* field exactly
>>> identical to their !crtc counterparts.
>>>
>>
>> I checked the flag options. There isn't any flag required. The only
>> reason to add this call is because cdns-dsi strictly requires the crtc_*
>> fields to be populated during the bridge enable.
>>
>> Secondly, if mode_fixup is deprecated, I think the crtc_atomic_check
>> would be the next best place to add this call.
> 
> That would be better, yes, but we shouldn't even have to do that in the
> first place. AFAIK all the path that create a drm_display_mode will call
> drm_mode_set_crtcinfo on it to fill those fields. So if they are missing
> somewhere, that's what the actual bug is, not something we should work
> around of at the driver level.
> 

You are right. This patch isn't required at all. The fix around the
mode->crtc_clock usage in one place in the cdns-dsi driver makes this
patch unnecessary. The DRM core does duplicate the timing parameters at
a later stage for the cdns-dsi bridge_enable to consume.

Dropped this patch in v2. Thanks!

Regards
Aradhya
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 94f8e3178df5..797ef53d9ad2 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -309,12 +309,23 @@  enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
 	return dispc_vp_mode_valid(tidss->dispc, tcrtc->hw_videoport, mode);
 }
 
+static
+bool tidss_crtc_mode_fixup(struct drm_crtc *crtc,
+			   const struct drm_display_mode *mode,
+			   struct drm_display_mode *adjusted_mode)
+{
+	drm_mode_set_crtcinfo(adjusted_mode, 0);
+
+	return true;
+}
+
 static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
 	.atomic_check = tidss_crtc_atomic_check,
 	.atomic_flush = tidss_crtc_atomic_flush,
 	.atomic_enable = tidss_crtc_atomic_enable,
 	.atomic_disable = tidss_crtc_atomic_disable,
 
+	.mode_fixup = tidss_crtc_mode_fixup,
 	.mode_valid = tidss_crtc_mode_valid,
 };